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

compare current version against latest and not wanted version in outdated command #4519

Merged
merged 6 commits into from
Oct 10, 2017
Merged

Conversation

danez
Copy link
Contributor

@danez danez commented Sep 22, 2017

Summary
current and wanted version might be the same but latest is a new major version
and as current and wanted are compared against each other it results in most outdated entries being white instead of the proper color

Here a before/after screenshot:

bildschirmfoto 2017-09-22 um 13 41 56

@danez danez changed the title change color against latest version and not wanted in outdated command compare current version against latest and not wanted version in outdated command Sep 22, 2017
@buildsize
Copy link

buildsize bot commented Sep 22, 2017

This change will increase the build size from 9.92 MB to 9.93 MB, an increase of 378 bytes (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 858.23 KB 858.21 KB -21 bytes (0%)
yarn-[version].js 3.78 MB 3.78 MB 228 bytes (0%)
yarn-legacy-[version].js 3.83 MB 3.83 MB 228 bytes (0%)
yarn-v[version].tar.gz 863.97 KB 863.99 KB 21 bytes (0%)
yarn_[version]all.deb 653.25 KB 653.18 KB -78 bytes (0%)

@BYK
Copy link
Member

BYK commented Sep 24, 2017

I don't have a strong opinion on this but I'd like to verify this behavior with @yarnpkg/core. Anyone willing to approve this patch?

@danez it would be great if you could add some tests for this btw.

@Haroenv
Copy link
Member

Haroenv commented Sep 25, 2017

What about adding a colour key on the bottom here, it might be hard to tell those apart otherwise

@kaylie-alexa
Copy link
Member

Thanks @danez! The code change looks fine, but I agree with @Haroenv that it's not as intuitive that the color changes between major vs minor versions.

@danez
Copy link
Contributor Author

danez commented Sep 25, 2017

Thanks for your feedbacks. I also noticed that in the above case it is also weird because all the changes in the screenshot are considered "major/breaking" (0.1.0 -> 0.2.0 or 1.0.0 > 2.0.0), though they still have different colors.

I guess the difference in the color should be:

  • red
    • You need to run yarn upgrade pkg-name --latest to update the package
    • Potentially breaking (according to semver)
  • yellow
    • If ranges are used in pkg.json, the package will be updated when running yarn run upgrade
    • Potentially not breaking (according to semver)

Does that make sense?

We could also add a --latest flag here to be inline with yarn upgrade, so by default only show in-range updates unless the flag is specified? Then the flag would decide if yarn compares against wanted or latest.

For the tests I could mock the formatter and check which color is used for which dep. Does that sound okay?

@danez
Copy link
Contributor Author

danez commented Sep 25, 2017

I also just noticed that upgrade-interactive uses the same coloring scheme that outdated uses which seems confusing to me. For example flow-bin is not in my range, similar to all other dependencies, but is yellow (see screenshot). So the most understandable difference between the colors for me is about is the upgrade in my current range (yellow) or not (red).

bildschirmfoto 2017-09-25 um 17 38 53

@kaylie-alexa
Copy link
Member

I also noticed that in the above case it is also weird because all the changes in the screenshot are considered "major/breaking" (0.1.0 -> 0.2.0 or 1.0.0 > 2.0.0), though they still have different colors.

(0.1.0 -> 0.2.0) is not considered major/breaking, since it's a minor bump and should be backwards compatible

I guess the difference in the color should be:

red
You need to run yarn upgrade pkg-name --latest to update the package
Potentially breaking (according to semver)
yellow
If ranges are used in pkg.json, the package will be updated when running yarn run upgrade
Potentially not breaking (according to semver)

I think it's confusing to have a color mean two things (how to upgrade and potentially breaking / non-breaking--the latter especially feels inconclusive). Ideally it should use the same key as upgrade-interactive

 "<red>"    : Major Update backward-incompatible updates
 "<yellow>" : Minor Update backward-compatible features
 "<green>"  : Patch Update backward-compatible bug fixes

For the tests I could mock the formatter and check which color is used for which dep. Does that sound okay?

Sounds good to me!

I also just noticed that upgrade-interactive uses the same coloring scheme that outdated uses which seems confusing to me.

I'm not sure I follow. The colors seem to correctly reflect the key in upgrade-interactive (major bumps being in red for backwards-incompatible change). The difference is that flow-bin is a minor bump, whereas the rest are major bumps.

@danez
Copy link
Contributor Author

danez commented Sep 27, 2017

@kaylieEB thanks for the clarification, seems I mixed some things up. I always thought that when the major version is 0 that semver defines that the minor version gets promoted to major, but that is not the case. The specification only says:

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

It seems I confused the semver spec with the way how the caret operator works, but that is not part of the specification.

Will try to finish this today.

current and wanted might be the same but latest is the new major version
which results in most outdated entries beeing white instead of the
proper color
@kaylie-alexa
Copy link
Member

@danez ping! need any help?

@danez
Copy link
Contributor Author

danez commented Oct 5, 2017

I think it is done now, I also added the legend.

@@ -114,6 +114,8 @@ const messages = {
allDependenciesUpToDate: 'All of your dependencies are up to date.',
legendColorsForUpgradeInteractive:
'Color legend : \n $0 : Major Update backward-incompatible updates \n $1 : Minor Update backward-compatible features \n $2 : Patch Update backward-compatible bug fixes',
legendColorsForOutdated:
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 is the same as legendColorsForUpgradeInteractive should I merge them? If yes what to name them?

Copy link
Member

Choose a reason for hiding this comment

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

yeah I'd prefer merging them. How about legendColorsForVersionUpdates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay changed :)

@kaylie-alexa
Copy link
Member

Thanks so much for your help @danez ! I'll merge once CI passes :)

@danez
Copy link
Contributor Author

danez commented Oct 10, 2017

Thank you :)

@kaylie-alexa kaylie-alexa merged commit 77f5e40 into yarnpkg:master Oct 10, 2017
joaolucasl pushed a commit to joaolucasl/yarn that referenced this pull request Oct 27, 2017
…ated command (yarnpkg#4519)

**Summary**
`current` and `wanted` version might be the same but `latest` is a new major version
and as current and wanted are compared against each other it results in most outdated entries being white instead of the proper color

Here a before/after screenshot:

![bildschirmfoto 2017-09-22 um 13 41 56](https://user-images.githubusercontent.com/231804/30743120-9efa6824-9f9c-11e7-9f17-7b511597e13b.png)
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.

4 participants