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

Provide a mechanism to compose type definitions #303

Closed
wants to merge 2 commits into from
Closed

Provide a mechanism to compose type definitions #303

wants to merge 2 commits into from

Conversation

isker
Copy link
Contributor

@isker isker commented Jan 5, 2017

This extends the syntax of the --type-add flag to allow including the globs of
other already defined types.

Fixes #83.

Hi @BurntSushi, this is pretty much a straight implementation of your spec from #83, except that we are also allowing Unicode numbers in type names in addition to Unicode letters. Letters were not enough for the default types (curse you, m4).

I am no rust expert so I'd appreciate specific feedback on my add_def changes (or anything, for that matter). And let me know if I missed any places to put tests or documentation (or if I put the documentation in too many places; it did feel a bit repetitive).

Thanks!

This extends the syntax of the --type-add flag to allow including the globs of
other already defined types.

Fixes #83.
for type_name in types {
let globs = self.types.get(type_name).unwrap().globs.clone();
for glob in globs {
self.add(name, &glob)?;
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, but this is going to fail CI because ripgrep's minimum version is currently Rust 1.12. You'll need to use try! here.

using the --type-add flag again:\n\n\
--type-add 'src:include:cpp,py,md' --type-add 'src:*.foo'\n\n\
Note that type names must consist only of Unicode letters or \
numbers. Punctuation characters are not allowed.");
Copy link
Owner

Choose a reason for hiding this comment

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

These docs are great. Can you also add them to the man page in doc/rg.1.md? (You'll need to cd doc && ./convert-to-man to actually produce the man file, which can be viewed with man -l rg.1. If you can't get pandoc installed easily, that's OK, I can do the final conversion step.)

Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Thank you so much! I have just a couple of minor nits, but this should otherwise be good to go. :D

@BurntSushi
Copy link
Owner

Oh, could you also add an integration test in tests/tests.rs? Thanks!

@isker
Copy link
Contributor Author

isker commented Jan 7, 2017

I was able to install pandoc but I could not view the man file. Probably some OSX incompatibility? Either way, you might want to take a look yourself.

@BurntSushi
Copy link
Owner

Merged in ed01e80

@BurntSushi BurntSushi closed this Jan 7, 2017
@BurntSushi
Copy link
Owner

Thanks! The doc formatting was a little off. I fixed that and also squashed the commits. Nice work!

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