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

version 100: first pass #2332

Merged
merged 2 commits into from
Oct 25, 2021
Merged

version 100: first pass #2332

merged 2 commits into from
Oct 25, 2021

Conversation

tiftran
Copy link
Contributor

@tiftran tiftran commented Oct 21, 2021

No description provided.

smallest_version = versions[0]
largest_version = versions[-1]

# check whether versions are continuous
Copy link
Contributor

Choose a reason for hiding this comment

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

One little thing: this comment talks about checking if the range is contiguous, but then the if statement and the 'then'-block are the non-contiguous case. It works fine, but I think it would read better in the other order (or if the comment was changed to talk about non-contiguous).

@tiftran tiftran changed the title first pass version 100: first pass Oct 25, 2021
@tiftran tiftran requested a review from mythmon October 25, 2021 17:59
Copy link
Contributor

@mythmon mythmon left a comment

Choose a reason for hiding this comment

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

Your commit messages and PR title just say "first pass". Do you have other work you're expecting to add here? It would be good to add some details that describe the change being made, at least to the commit messages. (amend and force push would be fine).

[
f'(env.version|versionCompare("{smallest_version}.!")>=0)',
f'(env.version|versionCompare("{largest_version}.*")<0)',
]
)

def get_capabilities(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need the capability for the versionCompare filter to be declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i took a peek at VersionRange and it had jexl.context.env.version for capabilities too, I would assume, Versions would need it as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this seems right. I'm not sure why it wasn't here before.

@tiftran
Copy link
Contributor Author

tiftran commented Oct 25, 2021

@mythmon eek, I assumed that I would've missed something considering this is my first time working on filters, therefore the initial commit was a "first pass" or "first attempt". Will be adding the capabilities to the filter.

Copy link
Contributor

@mythmon mythmon left a comment

Choose a reason for hiding this comment

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

You'll need to mark the filter as using non-baseline capabilities so that test passes, but besides that I think this looks good.

@tiftran
Copy link
Contributor Author

tiftran commented Oct 25, 2021

bors r= @mythmon

@bors
Copy link
Contributor

bors bot commented Oct 25, 2021

@bors bors bot merged commit b173d6e into mozilla:master Oct 25, 2021
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