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

Report actual version on the relaxed --version command. #49

Merged
merged 2 commits into from
May 13, 2018

Conversation

nikhilmitrax
Copy link
Contributor

This is better than reporting '0.0.1' for everything, or hardcoding it in
src/index.js since people might forget to update it on release.

This is better than reporting '0.0.1' for everything, or hardcoding it in
`src/index.js` since people might forget to update it on release.
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

Exactly what I thought about, good change 👍

@benperiton
Copy link
Member

benperiton commented May 12, 2018

Definitely a nice fix!
My only comment would be that using require('./package.json') to get the config object is possibly more portable if this package is ever required elsewhere (thinking path resolving) But can cross that bridge if needed 😄

@DanielRuf
Copy link
Contributor

I would also prefer require as @benperiton suggests as this is directly possible in Node.js.

Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

require(./package.json) is sufficient and preferred.

src/index.js Outdated
@@ -10,7 +10,7 @@ const converters = require('./converters.js')

var input, output

const version = JSON.parse(fs.readFileSync('./package.json')).version
const version = require('../package.json').version
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the now different path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The path looks for one folder above since (on my machine), the relaxed commands points to /usr/local/lib/node_modules/relaxedjs/src/index.js while package.json is located in /usr/local/lib/node_modules/relaxedjs/package.json.

I tested it locally to make sure if it works. Let me know if there are problems with this, and I will try to test it in more situations.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, the binary points to src/index.js
https://github.com/RelaxedJS/ReLaXed/blob/master/package.json#L7

@nikhilmitrax
Copy link
Contributor Author

nikhilmitrax commented May 12, 2018

changed as per request.

@Zulko Zulko merged commit 806ff21 into RelaxedJS:master May 13, 2018
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.

None yet

4 participants