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

Add isAtOrAbove and assertAtOrAbove #251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Panman82
Copy link

@Panman82 Panman82 commented Feb 9, 2021

Currently the assertAbove() function is incorrectly documented as a way to check for a minimum version and is actually a footgun if used in this way. (Ask me how I know... LOL)

Throws an error with the given message if a minimum version isn't met.

Instead, you must specify the version prior to the minimum version you are concerned about. This can also become an issue if there is a patch version of the prior version, which would then be unexpectedly accepted by the check. Ex: Ember Octane (v3.15) is the "true" minimum version, so you assertAbove('3.14.2'), but then v3.14.3 comes out...

This PR clarifies the documentation for assertAbove() and isAbove() but also adds assertAtOrAbove() and isAtOrAbove(), with documentation and testing. Basically the difference between the two is gt() vs gte().

@Panman82
Copy link
Author

Thinking more about this, after looking at what semver offers, another option is to just add an assert function for each version comparison function that this addon exports. That way the custom names isAbove and isAtOrAbove do not need to defined here. Rather, just use one of the semver function and prefix assert to the name to get that functionality.

PS. I think that one test scenario just needs to be re-run, it looks like there was some network issue when it booted up.

@Panman82
Copy link
Author

Reviewing my forks... Two years since I submitted this, any interest? With the move to peerDependancies and the embroider stuff, I'm not sure that this addon is even needed now. 🤷‍♂️

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.

1 participant