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

Relax thor version requirement #816

Closed
wants to merge 1 commit into from
Closed

Conversation

nertzy
Copy link
Contributor

@nertzy nertzy commented Feb 22, 2021

I use license finder on some Ruby projects that also use thor.

I want to be able to upgrade to thor 1.1 on my project, but license finder has a strict 1.0.x version requirement. This relaxes the requirement to 1.x.

@cf-gitbot
Copy link
Collaborator

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@david-a-wheeler
Copy link

+1 on relaxing the thor version specification. I have multiple dependencies that depend on thor; this license_finder requirement forces my application backwards and in the future it may not be possible.

Good luck!

@xtreme-shane-lattanzio
Copy link
Contributor

Hey @nertzy ! I am all for this but the issue comes with the tests. With the changes you made, it looks like some formats may have changed which caused the tests to fail. If you want to touch up the tests with this change we can take a look at how much output would change

@xtreme-shane-lattanzio
Copy link
Contributor

FYI I was playing around with thor today and there has been some changes that break LF. I created an issue here: rails/thor#751. Not sure of the plan to fix this but I think this type of thing is why the version of thor was more restrictive :\

@nertzy
Copy link
Contributor Author

nertzy commented Nov 1, 2021

It doesn't seem like thor is going to address this problem any time soon.

Would it make more sense to stop relying on thor's say and instead directly write to stdout?

@pivotal-cla
Copy link

@nertzy Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@nertzy Thank you for signing the Contributor License Agreement!

@clemens
Copy link

clemens commented Nov 23, 2021

Is there any intention of fixing this in LicenseFinder since Thor apparently won't change the behavior on their side? From what I can see, depending on Thor < 1.1.0 has the downside of effectively not supporting Ruby 3 (at least the Thor changelog says that 1.1.0 introduced Ruby 3 compatibility and the change to use URI.open indicates this as well). While definitely not everyone uses Ruby 3 already, it's the recommended Ruby version for both Rails 6.1 and 7.0.

@nertzy
Copy link
Contributor Author

nertzy commented Nov 23, 2021

I think it would be a great idea to use puts or something else simple to print to the console instead of say. If I get a free moment, I want to look into this.

I now have a project where I can not upgrade to the latest version of karafka because of the thor version restriction.

@srp-developers srp-developers force-pushed the upgrade-thor branch 5 times, most recently from 2398338 to 8739c6d Compare December 2, 2021 19:38
@nertzy
Copy link
Contributor Author

nertzy commented Jan 23, 2022

What can I do to get this merged?

@xtreme-shane-lattanzio
Copy link
Contributor

Hey @nertzy ! Sorry for the delay on this. I think you are right saying that this is how thor is now and we should probably do something about it. The change will be quite large but I imagine to be trivial to get say out of the code. If we can get the same output for license finder runs but using stdout or puts or something else, I am all for it and we can get the thor version relaxed. If you have time for this, I am willing to work through this with you because I do not have time to do this right now on my own. Let me know if you need help or questions and Ill let you know if I have any concerns. I really do appreciate you looking into this!

@xtreme-shane-lattanzio
Copy link
Contributor

I made a workaround to get this out so we didn't make major changes. #886 will update so the newest thor is used. Gonna close this for now!

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

Successfully merging this pull request may close these issues.

6 participants