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

apply selectors before checking for requirements #2973

Merged
merged 2 commits into from
Jul 4, 2018

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Jun 16, 2018

No description provided.

@@ -555,7 +555,10 @@ def find_used_variables_in_text(variant, recipe_text):
conditional_regex = r"(?:^|[^\{])\{%\s*(?:el)?if\s*" + v_regex + r"\s*(?:[^%]*?)?%\}"
# plain req name, no version spec. Look for end of line after name, or comment or selector
requirement_regex = r"^\s+\-\s+%s\s*(?:\s[\[#]|$)" % v_req_regex
all_res.extend([variant_regex, selector_regex, conditional_regex, requirement_regex])
if not selectors:
Copy link
Contributor

@msarahan msarahan Jun 25, 2018

Choose a reason for hiding this comment

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

This logic is counterintuitive to me. I think it means:

  • If selectors is True, it means you want to apply them to the recipe, rather than detect used variables in them
  • If selectors is False, you'll instead detect used variables in them, and not apply them.

Is that accurate? If so, do you mind adding a bit of a comment to clarify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should have been if selectors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please document how you want this to behave? I still don't trust myself to get this right, especially without some tests.

@msarahan
Copy link
Contributor

Aside from my one question, this looks fine. I'll be happy to merge if you can clarify things a bit for me.

@msarahan msarahan merged commit e2a4a8f into conda:master Jul 4, 2018
else:
recipe_text = (self.get_recipe_text(force_top_level=force_top_level,
apply_selectors=False).replace(
apply_selectors=apply_selectors).replace(
self.extract_outputs_text(apply_selectors=False).strip(), '') +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should have been apply_selectors=apply_selectors.

cc @CJ-Wright

else:
recipe_text = (self.get_recipe_text(force_top_level=force_top_level,
apply_selectors=False).replace(
apply_selectors=apply_selectors).replace(
self.extract_outputs_text(apply_selectors=False).strip(), '') +
self.extract_single_output_text(self.name(), apply_selectors=False))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

@github-actions
Copy link

Hi there, thank you for your contribution!

This pull request has been automatically locked because it has not had recent activity after being closed.

Please open a new issue or pull request if needed.

Thanks!

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Apr 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2022
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.

2 participants