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

Slightly adjust bootstrap regex #5745

Merged
merged 2 commits into from
Jan 24, 2022
Merged

Slightly adjust bootstrap regex #5745

merged 2 commits into from
Jan 24, 2022

Conversation

colinwm
Copy link
Contributor

@colinwm colinwm commented Jan 21, 2022

The bootstrap regex here is a big excessive, e.g. it matches paths like:

/src/bootstraps/settings.js

which is human writen code, whereas I think it was intended to match things like

/src/bootstrap-custom.js

I made it so that the wildcard (which currently doesn't match dots) also doesn't match slashes, which restrains it a bit.

@colinwm colinwm requested a review from a team as a code owner January 21, 2022 19:42
@look
Copy link

look commented Jan 21, 2022

Please add some tests to this section to cover these cases:

https://github.com/github/linguist/blob/80f3531e8a1014a23f4606458e5a528053ed3cac/test/test_file_blob.rb#L243-L245

@colinwm
Copy link
Contributor Author

colinwm commented Jan 21, 2022

@look good idea, done. Let me know if you have more test ideas

Copy link

@look look left a comment

Choose a reason for hiding this comment

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

I'm a little shocked those tests worked since sample_blob usually has a corresponding file, but I guess in this case it doesn't need to read the content.

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Oooof. Good catch. 🙇

@colinwm colinwm merged commit 73e2d73 into master Jan 24, 2022
@colinwm colinwm deleted the colinwm-fix-bootstrap branch January 24, 2022 17:08
@tclem tclem mentioned this pull request May 12, 2023
2 tasks
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.

None yet

3 participants