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

Minor doc-fix, proposed modification to "=" operator and addition of "=~" wrapper operator #121

Merged
merged 6 commits into from
May 11, 2020

Conversation

atreyasha
Copy link
Member

@atreyasha atreyasha commented May 9, 2020

1. Minor man-doc fix

  • c87b140: modification to man-doc to sort the hierarchies of man-pages

2. Minor fix assuming we keep the current = operator

  • c87b140...0a306a6: minor bugfix for current = operator in case regex does not match and vercmp does match. This could be the case where 8.2.33 would not regex match 8.2.0033, but it would still be a vercmp match.

  • 0a306a6...085b4b6: when inputting a (failing) version bound such as downgrade vim=1.2, the error message will state Unable to downgrade vim=1.2. I added a slight modification such that the error message would say Unable to downgrade vim to show that downgrade was able to parse the actual package.

3. Major fix assuming we modify the current = operator

I found it surprising (that = doesn't just mean =), but I suppose the unexpected behavior would result in multiple results and present the list to the user, so it's not particularly problematic.

I was thinking about your comment in #118 and came to realize that the = operator should indeed really mean equals and nothing else. To address this, I reverted the original = operator to have the correct meaning and added a new =~ operator which behaves exactly like that in bash (it just a wrapper for that). This will provide the user with the ability to conduct regex on package versions directly from the command line, for example downgrade "vim=~\.0033".

To address this, I chunked similar commits as per (2):

  • 085b4b6...0cbd8a3: revert = operator to equal meaning and add regex matching operator =~. Add respective tests for this as well.

  • 0cbd8a3...99f35d4: when inputting a (failing) version bound such as downgrade vim=~1.2, the error message will state Unable to downgrade vim=~1.2. I added a slight modification such that the error message would say Unable to downgrade vim to show that downgrade was able to parse the actual package.

Commits chunking

In this PR, (2) and (3) are mutually exclusive so we should choose what we like best. I would personally go for (3) since it addresses your comment in #118 and also provides the user with more flexibility in filtering packages while keeping the meaning of = to a hard equal.

If we like (2) more than (3), we can just drop the commits from 4868622 onward. If we like (3), we can just merge everything since the commits in (2) will be directly overriden.

I hope the chunking of commits helps in selecting what we find best. I tried to structure it as independently as possible so we can just pick and choose.

@atreyasha atreyasha requested a review from pbrisbin May 9, 2020 16:47
@atreyasha atreyasha force-pushed the as/operator-minorfix-enhancement branch from e321d03 to 99f35d4 Compare May 9, 2020 16:51
@atreyasha atreyasha changed the title Minor bugfix, proposed modification to "=" operator and addition of "=~" wrapper operator Minor doc-fix, proposed modification to "=" operator and addition of "=~" wrapper operator May 9, 2020
Copy link
Member

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

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

LGTM.

In the future, let's not update the rendered man page or update the locales files in PRs; it's just diff noise that slows down the GitHub UI :). I will do all that as part of release, along with the CHANGELOG, etc.

downgrade Show resolved Hide resolved
downgrade Outdated Show resolved Hide resolved
@atreyasha
Copy link
Member Author

LGTM.

In the future, let's not update the rendered man page or update the locales files in PRs; it's just diff noise that slows down the GitHub UI :). I will do all that as part of release, along with the CHANGELOG, etc.

Got it. I will edit the commits now just to ensure there are no conflicts for the other PR. Just to check, so for this PR you would prefer option (3) over option (2)? In that case, I can simplify the commits too.

@pbrisbin
Copy link
Member

for this PR you would prefer option (3) over option (2)?

Oh sorry, I forgot there were choices and just reviewed the PR as-is (which I guess is (3)?). I don't have a strong opinion, I'd defer to you. If pressed, I guess (3).

ordering see also's order in the man-page hierarchy from 8 to 1
@atreyasha atreyasha force-pushed the as/operator-minorfix-enhancement branch from 99f35d4 to d744577 Compare May 11, 2020 15:25
@atreyasha
Copy link
Member Author

atreyasha commented May 11, 2020

Reduced everything to 5 commits. Changes in locale and man-doc are now ommitted. If everything is fine, you can merge it.

@atreyasha atreyasha requested a review from pbrisbin May 11, 2020 15:26
@atreyasha atreyasha force-pushed the as/operator-minorfix-enhancement branch from d744577 to 2391e07 Compare May 11, 2020 15:47
adding regex operator to allow for = operator to only check for equality and not fuzzy matches
@atreyasha atreyasha merged commit 3332026 into master May 11, 2020
@atreyasha atreyasha deleted the as/operator-minorfix-enhancement branch May 11, 2020 17:38
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