search

codeclimate / codeclimate-grep

MIT License
2 stars 4 forks source link

Fix sample config #3

Closed wfleming closed 7 years ago

wfleming commented 7 years ago

Just set up this engine on app to dogfood a bit & I had to tweak a few things:

pbrisbin commented 7 years ago

Regarding the first two, I think the sample config was requested to look explicitly like that as a Product/UI/UX requirement. So I think the fix is not to change that, but to have the engine strip and escape as needed.

wfleming commented 7 years ago

Ok, I'll close this & leave digging into that behavior as a bug up to @pointlessone.

Will open a new tweak to address just the glob thing.

pointlessone commented 7 years ago

@pbrisbin I might have misread the requirements but I don't think that is intended. Delimiters imply that you can use other delimiters or that you can add flags after the closing delimiter. This is not the case here. README explicitly states that this engine uses GNU Extended Regular Expression syntax (links to the docs as well). It doesn't use delimiters, nor it allows alternate delimiters, or flags.

As for the other points, those are valid. I will handle those.

pbrisbin commented 7 years ago

@pointlessone It's very possible this PR is totally fine and the right fix. I don't know what the requirements were originally. I was just noting the fact that the config is the user-facing UX for this, so changes to it should be run by whoever's owning the Product side of things, and not done purely from a Technical perspective.

/cc @GordonDiggs

EDIT: and re-reading my earlier comment, I sounded more confident than I actually am, so I apologize.

pointlessone commented 7 years ago

@pbrisbin I will keep that in mind. Any input is valuable as I'm new to engine development.