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

Feature/improvements bugfixes #4

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

rejhgadellaa
Copy link

This MR:

  • Update npm transip 0.0.6
  • Load config.json, not config-example.json
  • Move config-example.json to ./config/
  • Add comments, whitespace, improve readability
  • Switch to console.log for output
  • Remove unused/-needed packages
  • Add package-lock.json
  • Add .editorconfig, .jshint
  • Removes docker support

@rejhgadellaa
Copy link
Author

Oops. This was meant to merge with my own fork. Not sure if you are interested in this MR, especially since it removes docker support (I just don't want to maintain that).

It does fix #2 and #3 ;)

@RolfKoenders
Copy link
Owner

Hee @rejhgadellaa!

Thank you so much for this PR! Im going to look into this. I see you updated some good stuff and added a editorconfig file. Im going to check the rules.

I do have some questions:

  • Why would we switch back to normal console output?
  • There is not much to support with Docker? For me its a requirement that i can run it as a Docker container.

I will do the code review when i have time and will let you know! And once again thank you so much for this PR! Appreciate the work and effort! 🙏🏻

Rolf

@rejhgadellaa
Copy link
Author

rejhgadellaa commented Mar 18, 2019

  1. I switched to console output because I prefer to see output in the terminal when I run something. Writing stdout into a file is easy enough and tools like foreverjs do it for you.

  2. It's just that I don't use docker so I can't test if I'm breaking anything. I figured it to be best to just remove the support.

Anyway, if you choose to cherry-pick things that make sense to you and leave out what doesn't, that's fine, too :)

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