search

mattludwigs / elm-version-manager

Manage local Elm versions: https://www.npmjs.com/package/elm-version-manager
29 stars 2 forks source link

Make it Right #3

Open mattludwigs opened 7 years ago

mattludwigs commented 7 years ago

In the spirit of "Make it work. Make it right. Make it fast" this code is currently in the make it work phase. We should make it right.

dennisreimann commented 7 years ago

Hey Matt, thanks for this project! I just thought about building something like this myself a few days ago as elmenv didn’t work for me. :)

Would you like contributions to issues like this one? If so, I'd be happy to help. (Could you elaborate on what would be required here from your point of view?)

inactivist commented 7 years ago

@mattludwigs I'm not sure what's needed to "make it right" -- I'll give it a spin and see what I can do to help.

mattludwigs commented 7 years ago

@dennisreimann and @inactivist, thank you for your interest in helping and I am always looking for help 👍. Sorry this issue was more of a mind dump I would not forget to add to it later. They are mostly either questions I would love input on, code style questions/fixes, and some known issues. I probably cannot get around to defining full issues until tomorrow, but somethings I were thinking of:

  1. Is doing the sym link to the files a bad idea? I know @inactivist you have already open a issue in regards to that
  2. Should we be trying to change the user's path on *nix based system?
  3. There are probably areas on string concatenation where they are repeated, so we should try to make those variables.
  4. The promise chain under the install command is not vary good and will create the folder path before downloading, so if a user does evm install 0.18.6 they will have that folder made.
  5. The error messages are not clear for in you try to use a version you don't have installed.
  6. Should there be a new directory src where we break the code up a little more?
  7. I am using double quotes (habit now that I am working in Elm and Elixir full time), and these should be changed to single quotes because the JS community prefers those.

@inactivist and @dennisreimann I would hold off until I get the Windows fix merged in or you will have merge conflicts. I plan on getting that in today or tomorrow early morning (I am mountain standard time in the US).

Disclaimer: I have been out of the JS game for a little bit now that I work with Elm and Elixir full time, so any JS input would be great. Just note I am trying to keep the JS a little old school to try to support Node v4+, so note the lack of ES6. And I mostly care about trying to keep the code simple and straight forward. Otherwise, any input is more than welcomed!