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

feat: make match against crate name case insensitive #69

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

LorenzSchueler
Copy link
Contributor

Some packages have an upper case name but their library has a lower case (but otherwise identical) name (e.g. Inflector). This means in the Cargo.toml the name is in upper case and the use statement uses the lower case name.
One option to fix this would be to convert the name to lower case before creating the regex expressions. However it is also possible that the package name is lower case and the lib name upper case (although I have not seen this in the wild).
This PR simply matches the crate name case insensitively and thereby fixes this problems in both cases.

Copy link
Owner

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, thanks! Can you tweak the comments to indicate that this will match insensitive to case, and add tests for the expected behavior in test_regexp, please? 🙏

@LorenzSchueler
Copy link
Contributor Author

I just added the positive tests to make sure they match in every place, but omitted the ones for the compound use as statements because the normal ones should be enough to make sure case insensitive matching works. If you would prefer having a case insensitive test variant of each test I'm happy to add them as well.
Also make_multiline_regexp now has a comment regarding case insensitive matching (make_line_regexp had it already).

Copy link
Owner

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, nice tests! (And sorry about the review delay.)

I think I'm going to merge this right now, but I'm wondering whether using the --with-metadata flag would have been sufficient, as the package name might indicate that the actual package was capitalized or something like that. In any case, it doesn't seem harmful to have this (except for a very bad edge case where two crates would use the same name but different casings, which... I truly hope doesn't exist).

Thanks again!

@bnjbvr bnjbvr merged commit 46c999d into bnjbvr:main Apr 6, 2023
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