Skip to content
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

Make order of rake gem:licenses deterministic #18

Merged
merged 1 commit into from
Feb 16, 2018

Conversation

edennis
Copy link
Contributor

@edennis edennis commented Feb 15, 2018

Hey there! First of all, thanks for publishing these rake tasks as a gem. It's been very useful for maintaining an overview of what software our project is using. In fact, we found it so useful that we've added them to our CI build to automatically check when new dependencies were added or removed and to notify us when this happens. Basically, what we've done is created a list of the licenses by running:

rake gem:licenses > LICENSES.md

And we've added a check to travis that does a diff between the version-controlled LICENSES.md and the current output of the rake task:

rake gem:licenses | diff -bu LICENSES.md -

The problem is that the order of the list isn't stable so we sometimes get false alarms. This PR addresses that and also adds an optional argument to the rake task to choose between alphabetical sorting of the list by license name or by the license count (default).

@olleolleolle
Copy link
Collaborator

olleolleolle commented Feb 15, 2018

Thanks for this thoughtful addition.

Idea for the title of this PR: Add "alphabetical" option to gem:licenses task

@edennis
Copy link
Contributor Author

edennis commented Feb 15, 2018

@olleolleolle Cheers! Yeah, the title could be better, I'll admit. The PR actually does two things:

  • Adds "alphabetical" option to gem:licenses task (as you suggested)
  • Make order of gem:licenses task deterministic

Happy to change the PR title and CHANGELOG.md entry if you think it makes sense.

@olleolleolle
Copy link
Collaborator

@edennis Make the change, be happy, and then I'll merge this change, since this change is 💯

@dblock
Copy link
Owner

dblock commented Feb 15, 2018

Before this gets merged, can we think about future proofing this? I am worried that we can end up with 10 options to Rake, where order matters. I'm also worried that tomorrow someone will want a different type of sort and that we're comparing strings.

Maybe we can achieve the same results with just adding | sort to the command line? Or am I missing something?

Otherwise I would want to add a command line tool to this gem and stop recommending using rake (see https://github.com/dblock/frgom for an example) that takes a --sort option instead.

I am not saying don't merge this PR, just thinking about where to take this.

@edennis edennis changed the title Make sort order stable (by count or alphabetically) Make order of rake gem:licenses deterministic Feb 15, 2018
@edennis
Copy link
Contributor Author

edennis commented Feb 15, 2018

@dblock I can understand your concerns and have adapted the PR to only make the sort order of the rake gem:licenses task deterministic. That will fix my issue and not change any public interfaces. Unfortunately, just piping the output through sort doesn't work since the gems are grouped by license.

+1 to replacing the rake tasks with a CLI. Passing args to rake really leaves a lot to be desired.

@dblock dblock mentioned this pull request Feb 16, 2018
@dblock
Copy link
Owner

dblock commented Feb 16, 2018

Great, I'm merging this as is. Added #19 to add CLI. This is a nice little project for someone who wants to contribute!

@dblock dblock closed this Feb 16, 2018
@dblock dblock reopened this Feb 16, 2018
@dblock dblock merged commit 0b9c320 into dblock:master Feb 16, 2018
@dblock
Copy link
Owner

dblock commented Feb 16, 2018

@edennis Want to help out? You could do #19 and join the maintainers? ;)

@edennis
Copy link
Contributor Author

edennis commented Feb 16, 2018

@dblock Thanks for the offer! I won't make any promises I can't keep but would prefer to pleasantly surprise instead. ;-)

In the meantime, do you think there's any chance at cutting the 0.2.2 release?

@dblock
Copy link
Owner

dblock commented Feb 16, 2018

Done @edennis

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

Successfully merging this pull request may close these issues.

3 participants