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

Improve package version detection using jq, python and node #441

Merged
merged 9 commits into from
Nov 21, 2018

Conversation

salmanulfarzy
Copy link
Member

@salmanulfarzy salmanulfarzy commented May 20, 2018

grep solution to fetch version package.json was hacky and wasn't always returning expected values. Now using other fallback methods to get version information, Following discussions from #439

Changes

  • Fetch version information with jq with fallback to python and node.
  • Gracefully exit from the section if no version information is found or is undefined from package.json

Fix #439
Close #552
Close #553

@salmanulfarzy salmanulfarzy added the bug Bug related to code base, behavior, displaying, etc. label May 20, 2018
@salmanulfarzy salmanulfarzy self-assigned this May 20, 2018
sections/package.zsh Outdated Show resolved Hide resolved
@salmanulfarzy salmanulfarzy changed the title Only display package version prefixed with two spaces Improve package version detection using jq and grep Jul 21, 2018
@maximbaz maximbaz mentioned this pull request Nov 9, 2018
@salmanulfarzy
Copy link
Member Author

This returns the top level version information from package.json and won't consider version information nested inside othres.

This works,

https://github.com/denysdovhan/spaceship-prompt/blob/08361383e91bc40fb30bdb223377ba796e04dc90/package.json#L1-L4

This won't, (from #552)

native script version


@Darkhogg It would be better if you leave a comment while down voting things, So we could figure out what we missed and resolve that.

@maximbaz
Copy link
Contributor

maximbaz commented Nov 9, 2018

The fact that grep solution looks for nested versions is the actual bug, that's why I was saying that there's no place for grep approach in this section.

Here's the full package.json from the picture you shared, this section must have retrieved only parent version: https://github.com/NativeScript/sample-Groceries/blob/master/package.json

@Darkhogg
Copy link
Contributor

Darkhogg commented Nov 9, 2018

@salmanulfarzy I believe using grep as the only fallback is silly, as there are plenty of other ways of getting the version in a correct way (i.e.: parsing JSON) that will work for most people, if not everyone.

I discussed alternatives and my opinion on this topic in #439.

Copy link
Contributor

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

As a practical suggestion: I think you could avoid some controversy by reverting the grep regex exactly back to what it was before (the user would then need to install jq to fix their bug). That would make this PR strictly better than what's on trunk, although you would have to accept that the jq and grep code paths may differ in behavior.

Or, you could land as-is (or with my suggested regex switch), in which case I still think this is better than the state of master. But this risks breaking other cases.

You could always debate a follow-up PR to swap in node as the fallback option. But please should check spaceship::exists node to be safe.

sections/package.zsh Outdated Show resolved Hide resolved
sections/package.zsh Show resolved Hide resolved
@salmanulfarzy
Copy link
Member Author

why I was saying that there's no place for grep approach in this section.

using grep as the only fallback is silly, as there are plenty of other ways of getting the version in a correct way (i.e.: parsing JSON) that will work for most people,

Suggestion by @Darkhogg to use python and node as fall backs sounds good. Dropping that in favor of current methods.

Left notes on Options and Troubleshooting page regarding potential performance issues.

this section must have retrieved only parent version: https://github.com/NativeScript/sample-Groceries/blob/master/package.json

New changes works on this file.

@salmanulfarzy salmanulfarzy changed the title Improve package version detection using jq and grep Improve package version detection using jq, python and node Nov 10, 2018
sections/package.zsh Outdated Show resolved Hide resolved
sections/package.zsh Outdated Show resolved Hide resolved
Runrioter and others added 2 commits November 10, 2018 13:12
Co-Authored-By: salmanulfarzy <salmanulfarzy@gmail.com>
Co-Authored-By: salmanulfarzy <salmanulfarzy@gmail.com>
Copy link
Contributor

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

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

Awesome 👍

Copy link
Member

@denysdovhan denysdovhan left a comment

Choose a reason for hiding this comment

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

Looks OK for now. I think you can merge it @salmanulfarzy.

However, I have a few doubts about this section overall. This section was built for showing package version, speaking about any package manager and not only npm. Right now it tries to parse JSON (because we know that package.json is JSON). Nevertheless, not every package manager uses JSON, some of them uses YAML, TOML, etc syntax.

I'm thinking about moving this section out of the Spaceship and distribute as an optional third-party section.

docs/Options.md Outdated Show resolved Hide resolved
@nfischer
Copy link
Contributor

I'm thinking about moving this section out of the Spaceship and distribute as an optional third-party section.

@denysdovhan What's spaceship's API to add in third-party modules? I filed #318 a while back because I felt support was lacking--but let me know if things have changed since then.

Copy link
Member

@denysdovhan denysdovhan left a comment

Choose a reason for hiding this comment

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

Looks better now.

@salmanulfarzy
Copy link
Member Author

Commits are mess in this now, Will merge after fixing that 😄

@salmanulfarzy salmanulfarzy merged commit ece7de8 into spaceship-prompt:master Nov 21, 2018
salmanulfarzy added a commit to salmanulfarzy/spaceship-prompt that referenced this pull request Nov 23, 2018
* origin/4.0: (44 commits)
  3.8.0
  Support multiple files for COMPOSE_FILE in docker (spaceship-prompt#395)
  Add Terraform Workspace section (spaceship-prompt#542)
  Document kubecontext showing namepace (spaceship-prompt#566)
  Use Zsh field splitting instead of awk (spaceship-prompt#544)
  Add namespace to k8s section (spaceship-prompt#565)
  3.7.2
  Document go.mod detection in golang section
  Improve package version detection using jq, python and node (spaceship-prompt#441)
  Truncate .git directory and childs like normal directories (spaceship-prompt#417)
  Add option to toggle verbose rust version (spaceship-prompt#521)
  Change font suggestion: Fira Mono → Fira Code
  Support go.mod files and fix GOPATH matching (spaceship-prompt#559)
  Hide PR template guidance behind comments (spaceship-prompt#556)
  Document pyproject.toml in pyenv section (spaceship-prompt#551)
  3.7.1
  Recognize pyproject.toml when showing pyenv section (spaceship-prompt#550)
  Fix orderd list count on CONTRIBUTING page (spaceship-prompt#548)
  Add image in doc for powerline dir lock symbol (spaceship-prompt#517)
  Remove argument from char test shebang (spaceship-prompt#533)
  ...
dedene pushed a commit to zenjoy/spaceship-prompt that referenced this pull request Feb 24, 2019
…p-prompt#441)

Co-Authored-By: Daniel Escoz <darkhogg@gmail.com>
Co-Authored-By: Runrioter Wung <runrioter@qq.com>
Co-Authored-By: Nate Fischer <nfischer@users.noreply.github.com>
denysdovhan pushed a commit that referenced this pull request May 27, 2021
Co-Authored-By: Daniel Escoz <darkhogg@gmail.com>
Co-Authored-By: Runrioter Wung <runrioter@qq.com>
Co-Authored-By: Nate Fischer <nfischer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug related to code base, behavior, displaying, etc.
Development

Successfully merging this pull request may close these issues.

6 participants