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.

Sign up for GitHub

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

Jump to bottom

feat: check node engines version in package.json #1847

Merged
merged 4 commits into from
Dec 5, 2020
Merged

feat: check node engines version in package.json #1847

merged 4 commits into from
Dec 5, 2020

Conversation

t-mangoe
Copy link
Contributor

@t-mangoe t-mangoe commented Nov 1, 2020

Description

I added codes which check node.js engines version written in package.json

Motivation and Context

Closes #1605

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
    -> I added two tests.

@t-mangoe t-mangoe changed the title check node engines version in package.json feat: check node engines version in package.json Nov 1, 2020
andytom
andytom reviewed Nov 3, 2020
View reviewed changes
Copy link
Member

@andytom andytom left a comment

Choose a reason for hiding this comment

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

Hi @t-mangoe thank you for your contribution. I have added a few suggestions hopefully nothing major.

Can you also update the Docs for the NodeJS module to include this additional configuration (you only need to update the English docs all translations are done via translate.starship.rs).

Cargo.toml Outdated
@@ -30,16 +30,16 @@ tls-vendored = ["native-tls/vendored"]
[dependencies]
clap = "2.33.3"
ansi_term = "0.12.1"
dirs-next = "2.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert the downgrades to these libraries?

src/modules/nodejs.rs Outdated Show resolved Hide resolved
src/modules/nodejs.rs Outdated Show resolved Hide resolved
src/modules/nodejs.rs Outdated Show resolved Hide resolved
@t-mangoe
Copy link
Contributor Author

t-mangoe commented Nov 7, 2020

@andytom Thank you for your review.
Following your suggestions, I have fixed my codes.
Also, I updated the Docs for the NodeJS module.

@andytom
Copy link
Member

andytom commented Nov 15, 2020

@andytom Thank you for your review.
Following your suggestions, I have fixed my codes.
Also, I updated the Docs for the NodeJS module.

Hi @t-mangoe, Sorry for the delay in replying. Thanks for fixing the issues unfortunately it looks like there are still some conflicts with the Cargo.toml if you can fix those conflicts I think we should be good to go here.

@t-mangoe
Copy link
Contributor Author

@andytom
Oh, sorry. I overlooked the conflicts in Cargo.toml.
I resolved these conflicts.

@t-mangoe
Copy link
Contributor Author

t-mangoe commented Dec 5, 2020

I have resolved conflicts.

@andytom andytom merged commit 89588a7 into starship:master Dec 5, 2020
@andytom
Copy link
Member

andytom commented Dec 5, 2020

Thank you for your contribution @t-mangoe

chipbuster pushed a commit to chipbuster/starship that referenced this pull request Jan 14, 2021
@t-mangoe @chipbuster
feat(nodejs): check node engines version in package.json (starship#1847)
4f11ef5 
* check node engines version in package.json

* fix code, following review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andytom andytom andytom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show node version in red when incompatible with the current package
2 participants
@t-mangoe @andytom