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

feat(system) add os and version to System class #602

Merged
merged 1 commit into from
Feb 4, 2018

Conversation

aileen
Copy link
Member

@aileen aileen commented Jan 30, 2018

no issue

  • added a new getter to the System class, that returns the operating system as well as the version
  • works for linux, mac, and windows. Everything else falls back to the native os.platform() fn.
  • prints the os and the version in the debug information
  • added tests

@coveralls
Copy link

coveralls commented Jan 30, 2018

Coverage Status

Coverage decreased (-0.1%) to 98.518% when pulling 6fac7de on AileenCGN:add-os-info into ca68049 on TryGhost:master.

@acburdine
Copy link
Member

Out of curiosity, is there a reason we're using our own commands and not something like https://www.npmjs.com/package/systeminformation ? imo using a library for it is probably a better approach here.

@aileen aileen changed the title feat(system) add os and version to System class [HOLD] feat(system) add os and version to System class Feb 1, 2018
@aileen aileen added the later label Feb 1, 2018
@aileen
Copy link
Member Author

aileen commented Feb 1, 2018

Thanks for the info. I will look into it, but with using systeminformation we'd need to refactor the code to be async.

If we decide to keep our own commands, I will source the fn out into a util, but still keep the command accessible from our System class.

@acburdine
Copy link
Member

acburdine commented Feb 1, 2018

Sounds good to me to use our own commands - any refactoring can be done as part of addressing #540

@aileen aileen changed the title [HOLD] feat(system) add os and version to System class feat(system) add os and version to System class Feb 4, 2018
@aileen aileen removed the later label Feb 4, 2018
@aileen
Copy link
Member Author

aileen commented Feb 4, 2018

@acburdine This is ready for review!

no issue

- added a new getter to the System class, that returns the operating system as well as the version
- works for linux, mac, and windows. Everything else falls back to the native `os.platform()` fn.
- prints the os and the version in the debug information
- added tests
- updated issue template
Copy link
Member

@acburdine acburdine left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@acburdine acburdine merged commit bd24652 into TryGhost:master Feb 4, 2018
@aileen aileen deleted the add-os-info branch May 10, 2018 01:50
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.

3 participants