New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable cmd line support for text min #26

Closed
sayedihashimi opened this Issue Jul 16, 2014 · 7 comments

Comments

Projects
None yet
2 participants
@sayedihashimi
Member

sayedihashimi commented Jul 16, 2014

We should add cmd line similar to how we added support to call the image optimizer support being called from the command line in #22.

@philoushka

This comment has been minimized.

Show comment
Hide comment
@philoushka

philoushka Jul 31, 2014

Contributor

The image compressor and minifier are quite similar in the tasks that they do. They both follow the same basic pattern. I find myself tackling this and the easiest thing is to copy and paste from Img's Program.cs into Minifier's Program.cs. Suggest perhaps:

  • creating a base class in Common project
  • moving implementation out of Program.cs in each proj into a new class that handles all this; Program.cs should really just be the in and out, and it should call out to a class that does all the work for the project. (should be, right.).
  • have each of those new classes inherit from this proposed new base class that would house common functionality and be accessible to both. Result: less duplication of code. e.g. QueueExistingFiles() and EnsureCacheExists()
  • some implementations are different, really, so those methods belong in an interface that would be implemented. e.g. ProcessQueue() and AddToQueue().

Thoughts?

Contributor

philoushka commented Jul 31, 2014

The image compressor and minifier are quite similar in the tasks that they do. They both follow the same basic pattern. I find myself tackling this and the easiest thing is to copy and paste from Img's Program.cs into Minifier's Program.cs. Suggest perhaps:

  • creating a base class in Common project
  • moving implementation out of Program.cs in each proj into a new class that handles all this; Program.cs should really just be the in and out, and it should call out to a class that does all the work for the project. (should be, right.).
  • have each of those new classes inherit from this proposed new base class that would house common functionality and be accessible to both. Result: less duplication of code. e.g. QueueExistingFiles() and EnsureCacheExists()
  • some implementations are different, really, so those methods belong in an interface that would be implemented. e.g. ProcessQueue() and AddToQueue().

Thoughts?

@sayedihashimi

This comment has been minimized.

Show comment
Hide comment
@sayedihashimi

sayedihashimi Jul 31, 2014

Member

Yeah the code is not currently factored very well. We have a lot of duplication. I agree that we should def consolidate if we can. The suggestions you suggest above all look really good to me.

If we can get the text min working as a stand alone exe I can make a self bootstrapping script, PowerShell and if anyone wants MSBuild, to make it easier to use.

Member

sayedihashimi commented Jul 31, 2014

Yeah the code is not currently factored very well. We have a lot of duplication. I agree that we should def consolidate if we can. The suggestions you suggest above all look really good to me.

If we can get the text min working as a stand alone exe I can make a self bootstrapping script, PowerShell and if anyone wants MSBuild, to make it easier to use.

@philoushka

This comment has been minimized.

Show comment
Hide comment
@philoushka

philoushka Jul 31, 2014

Contributor

So it turns out you've pulled in a change that wasn't in my original PR.
Silly me, working in and commiting to master. You've actually pulled changes to TextMinifier\Program.cs that I had just committed and was confusing on how to make another PR from the same branch. Lesson learned: make a new branch per feature or PR.

I didn't realize that subsequent commits to a branch after a PR is made would be included if the PR is accepted.

So this issue has been solved basically by a copy of the ImgCompressor's implementation, copy paste over to TextMinifier.

It'll output via help switch the exact same params as ImgCompress, even though they're not used.

min

Contributor

philoushka commented Jul 31, 2014

So it turns out you've pulled in a change that wasn't in my original PR.
Silly me, working in and commiting to master. You've actually pulled changes to TextMinifier\Program.cs that I had just committed and was confusing on how to make another PR from the same branch. Lesson learned: make a new branch per feature or PR.

I didn't realize that subsequent commits to a branch after a PR is made would be included if the PR is accepted.

So this issue has been solved basically by a copy of the ImgCompressor's implementation, copy paste over to TextMinifier.

It'll output via help switch the exact same params as ImgCompress, even though they're not used.

min

@sayedihashimi

This comment has been minimized.

Show comment
Hide comment
@sayedihashimi

sayedihashimi Jul 31, 2014

Member

Should we revert the change or leave it for now?

Member

sayedihashimi commented Jul 31, 2014

Should we revert the change or leave it for now?

@philoushka

This comment has been minimized.

Show comment
Hide comment
@philoushka

philoushka Jul 31, 2014

Contributor

Let's keep it. Should be all good and functional. I meant to include it as you've accepted it (copy/pasted from ImgCompress), with the intent to refactor later with a base class and interface, etc later.

On Jul 30, 2014, at 22:50, Sayed Ibrahim Hashimi notifications@github.com wrote:

Should we revert the change or leave it for now?


Reply to this email directly or view it on GitHub.

Contributor

philoushka commented Jul 31, 2014

Let's keep it. Should be all good and functional. I meant to include it as you've accepted it (copy/pasted from ImgCompress), with the intent to refactor later with a base class and interface, etc later.

On Jul 30, 2014, at 22:50, Sayed Ibrahim Hashimi notifications@github.com wrote:

Should we revert the change or leave it for now?


Reply to this email directly or view it on GitHub.

@sayedihashimi

This comment has been minimized.

Show comment
Hide comment
@sayedihashimi

sayedihashimi Jul 31, 2014

Member

Ok sounds good, thanks.

Member

sayedihashimi commented Jul 31, 2014

Ok sounds good, thanks.

@sayedihashimi

This comment has been minimized.

Show comment
Hide comment
@sayedihashimi

sayedihashimi Dec 18, 2014

Member

I think we can close this, please re-open if we still need it.

Member

sayedihashimi commented Dec 18, 2014

I think we can close this, please re-open if we still need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment