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

Fixes and Improvements #43

Merged
merged 13 commits into from
Jan 12, 2025
Merged

Fixes and Improvements #43

merged 13 commits into from
Jan 12, 2025

Conversation

plucia-mitre
Copy link
Contributor

First, thanks for a very neat tool!

Secondly, sorry for a large-ish combined pull request right before the holidays! There's no rush on this, I hope you're having a joyous time! I can also easily split this out into smaller pull requests if you'd like, just let me know! I developed these changes in a bit different order, and then afterwards rearranged git to make a bit more of a logical story, but the individual commits of this pull request haven't been tested in the order they are here, though the overall result has been tested.

This pull request addresses a number of things:

  • Cleans up some extra error logging that was occurring by moving it it a trace log level.
  • Fixes a corner case where licensure failed to run on files that were not in git when attempting to automatically determine licensing date. With this fix, such files now default to the current year.
  • Print the number of files that are about to be printed in various outputs of the --check option.
  • Ignore symlinks. Encountering a symlink file broke licensure prior to this fix.
  • Add a "replaces" feature which allows automatic replacing of existing license texts that do not match the current license template. This is very handy if migrating from a proprietary license to an open source license for example.
  • Allows limiting what files a commenter will run on. This feature was useful to allow for different commenting formats in different markdown files located in different directories.

Happy to answer any questions about this!

@chasinglogic
Copy link
Owner

Hey @plucia-mitre just wanted to say that I'm taking a look at this this week if not today.

Thanks for the pre-emptive patience with the holidays.

src/config/license.rs Show resolved Hide resolved
src/licensure.rs Outdated Show resolved Hide resolved
src/licensure.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@chasinglogic
Copy link
Owner

I've submitted the first round of changes for the small stuff so you get some early feedback.

Due to the size of the PR I haven't reviewed the new replaces feature yet as that will take more time but will do so as soon as I'm able.

src/licensure.rs Outdated Show resolved Hide resolved
@chasinglogic
Copy link
Owner

AFAICT these new features don't have new tests. That will be a requirement before this gets merged.

@plucia-mitre
Copy link
Contributor Author

Thanks for looking at all this! The feedback was helpful! I just pushed a commit with a few fixes, documentation updates, and test cases. I've also left a bunch of comments on your discussions above. Let me know if you'd like me to resolve those that I've addressed or if you'd like to do that after further review. Thanks again!

@chasinglogic
Copy link
Owner

@plucia-mitre I've gone through and either responded or resolved comments as appropriate.

Once the last few changes are made I'm happy to merge this, let me know if anything is unclear.

@plucia-mitre
Copy link
Contributor Author

Thank you @chasinglogic ! I believe I've made all the remaining fixes besides the matrix suggestion (which I've commented about above). Once I understand that better I can try to implement it if you'd like, or you can merge if you're happy enough with how it is now. I do have a few other (simpler) MR's queued up for a few other things after this one.

@chasinglogic chasinglogic merged commit f09d5e3 into chasinglogic:master Jan 12, 2025
5 checks passed
@plucia-mitre
Copy link
Contributor Author

Thank you!!!

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

Successfully merging this pull request may close these issues.

2 participants