shama / napa

:wine_glass: A helper for installing stuff without a package.json with npm.
MIT License
412 stars 34 forks source link

Add options, fix a bug and add some outputs #37

Closed msieurtoph closed 9 years ago

msieurtoph commented 9 years ago
  1. Adding two options (README.md is updated) :
    • cache (use cache or not)
    • force (force resinstall or not)
  2. When using napa username/repo, the repo will be ignored if a package.json exists in cwd. I fixed that.
  3. I added some outputs to tell if repo is ignored or reinstalled. Track log.info() statements to see what I have add.

Hope you'll like it.

I did not add test statements, because I only know jasmine by now and I did not recognize the langage you used. Tell me and I will try to write some.

shama commented 9 years ago

Thanks for the PR! This looks good but could you check on the CI errors above?

The test library I'm using is tape and you can run the tests locally with npm test. Let me know if you have any questions writing tests with it. Thanks!

msieurtoph commented 9 years ago

Oh, the readme update started new builds...

I am currently commiting a new version, including :

Tape tests looked ok locally. Hope this will be good now.

++

Note: I did not figure how to test pkg and force options out because they do not involve specific methods. If you have an idea ...

msieurtoph commented 9 years ago

Hold on before merging.

I will fix this one too: https://github.com/shama/napa/issues/12, (at least, I will try) ;)

shama commented 9 years ago

Thanks for your work! Let's try and keep the PR focused on the force and cache options and avoid too much refactoring.

Also what is the use case for the pkg option? I'm a little confused by it's function.

msieurtoph commented 9 years ago

I agree, pkg option is not that obvious.

The point was about dev or test phases especially.

Actually, if we call napa.cli('user/repo'). It will systematically check the package.json and add its repos to the todo list. Even if we don't want/need to. The --no-pkg option is just a flag to tell napa.cli to only deal with the argument-repos ... and ignore package.json.

I can remove it. It seemed useful to me at the time, but I figure out now it is really confusing nad not so useful...


I am fixing the https://github.com/shama/napa/issues/12 because I need it for production. In my case, the pb comes from the port in the hostname. It introduces an additionnal : that drives all if-tests wrong. I will use the url-parse that will do the job very simpler.

I will let you know...

msieurtoph commented 9 years ago

Hi Shama,

I understand you do not want to merge this PR. I thought fixing https://github.com/shama/napa/issues/12 and adding options mechanism implied a big refactoring of your code. By the way, it contains some bugs I have found after pushing it.

I took into account the comments you made about my work (especially concerning exposed APIs and the "keep it simple" line). And even if I do not agree about everything, I must admit it made sense to me and helped me to think different. But I failed adding that much enhancements without refactoring too much. Since I needed to change the arguments parsing, it implied a lot of changes, at least in the client part.

Needing this evolution for production (fix for https://github.com/shama/napa/issues/12), I finally published my own package on github and npm (named any-packages). It let me refactor as I want and change whatever I need without restriction.

Mainly, I kept the mechanism of having a client (cli.js) and a package constructor (pkg.js) which was great. I also kept the cache functionnality and the "extra property" in the package.json to list the repos to download. However, I abandoned the github cloning to only process pure downloads (all github repos can be downloaded, using the master archive, instead of being cloned) and I completely changed the way the options and args are managed and injected to the package constructor. I also gave the final callback a way to get some info about installation for each package.

To cut a long story short, it is largely inspired by yours, sometimes very very similar, sometimes definitively not. And I hope you do not mind about that. Far be it from me to take the credits for your ingenious idea. I put a reference to your package in the first lines of my README.md. If you want me to highlight it more, just tell.

About napa, I am still motivated to help you maintain it. If you are still open to receive PR, we can delete this one and start over, doing it your way and focusing on a single problem at the time (first: adding options; then we will see for the rest). It is up to you, let me know ...

shama commented 9 years ago

@msieurtoph :+1: Cool that works. :) any-packages looks good (and a better name heh). Always good to have more library options available. Thanks for the mention and I appreciate you trying to help me out here.