-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
chore: added nvmrc #582
chore: added nvmrc #582
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
@fmvilas @derberg @ron-debajyoti kindly review the PR |
I'm not a maintainer tbh, just another fellow contributor 😄 @jonaslagoni |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ritik307 Thanks for taking the time to do this 👍
One question, is there any reason you see us needing the script nvmrc
rather than just committing that file? That way the steps others need to do it only nvm use
🤔
I think the implemented approach is better bcz there won't be a need for a dev to have nvm already installed in their machine and also if a dev doesn't require nvmrc file they still have to keep it in the root folder regardless. I don't have any problem switching to the way you are thinking to implement this I will update the PR accordingly. |
Regardless of this approach or the other, would they not need The only difference I see is that, with your script, we can "dynamically` create the appropriate nvmrc file. However, as the node requirement wont really change, having it hardcoded might be a better fit 🙂 |
Done! kindly review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a few follow up questions 🙂
Also for the title of the PR, please use chore:
as this is not a feature for the library, but a feature for the development setup 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
/rtm |
Pull Request Test Coverage Report for Build 1698990891
💛 - Coveralls |
Kudos, SonarCloud Quality Gate passed! |
@all-contributors please add @ritik307 for doc |
I've put up a pull request to add @ritik307! 🎉 |
🎉 This PR is included in version 0.44.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.0.0-next.23 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
.nvmrc
.docs/development.md
with the commands to setup.nvmrc
Related issue(s)
Resolves #575