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

metadata: Fixed selector regex to avoid confusion with jinja templates #957

Merged
merged 1 commit into from
May 16, 2016

Conversation

stuarteberg
Copy link
Contributor

The fix here is to replace (.+) with ([^\[\]]+):

-sel_pat = re.compile(r'(.+?)\s*(#.*)?\[(.+)\](?(2).*)$')
+sel_pat = re.compile(r'(.+?)\s*(#.*)?\[([^\[\]]+)\](?(2).*)$')

That is, selectors don't contain "any character", they contain "any non-bracket character".

Fixes #955

@msarahan
Copy link
Contributor

Thanks @stuarteberg - I have been fighting regexes on this one for a while this morning. Thanks for putting and end to that.

I think a better place to test (or perhaps in addition to this) would be test_metadata.py, in test_select_lines. I had added:

{{ environ["test"] }}  # [abc]

there. The tests you added are fine - but adding this bit in test_metadata is a more direct test of this regex. Otherwise, the only failure on travis was flake8.

@stuarteberg
Copy link
Contributor Author

stuarteberg commented May 16, 2016

I have been fighting regexes on this one for a while this morning.

Yeah, the current PR was not my first attempt. I had a different PR written up that changed the regex entirely, but then I had a head-smacking "ah-ha" moment before I hit the "Create Pull Request" button.

in test_select_lines. I had added:

{{ environ["test"] }}  # [abc]

The current PR already adds a few equivalent cases to test_select_lines(), but I'll add that one as well.

Otherwise, the only failure on travis was flake8.

Oops, I'll fix that, too.

@stuarteberg stuarteberg force-pushed the fix-selector-jinja-combo branch from 28ba6c4 to 7de6a07 Compare May 16, 2016 18:34
@msarahan
Copy link
Contributor

Very nice, thanks @stuarteberg

@msarahan msarahan merged commit e8e83de into conda:master May 16, 2016
@stuarteberg
Copy link
Contributor Author

regex

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression regarding [] in jinja
2 participants