JSRocksHQ / harmonic

The next static site generator
http://harmonicjs.com/
MIT License
282 stars 26 forks source link

Enhancement to enable running harmonic commands from subfolders #139

Closed soapdog closed 9 years ago

soapdog commented 9 years ago

This pull request enables running the "build", "new_post", "new_page" and "run" commands from anywhere inside an harmonic site.

It works by creating a new findHarmonicRoot() helper function that will look into the current folder for harmonic.json and if not found it will start climbing to the parent folders until it finds it. Once the config file is found it returns it. If the config file is not found anywhere it displays a friendly error message and throws an error.

This leads to a better user experience because they don't need to return to the folder where harmonic.json is located before running any command.

UltCombo commented 9 years ago

Very nice! I'll test and merge this tonight. Merging this will close #111

jaydson commented 9 years ago

Good job @soapdog !

UltCombo commented 9 years ago

@soapdog currently, your PR only calls findHarmonicRoot in the default parameter value, that is, only when the user does not supply a path.

I believe it would make more sense to call findHarmonicRoot(path) inside the function so that it works for both supplied and default parameters.

Also, going the extra mile, it would be really nice to call findHarmonicRoot() inside the actual functions that handle the command's logic (e.g., core's build function, util's config function) so that it would work for both CLI and API usage. As this part is rather unorganized, I can handle that personally later if anything.

And I'd like to avoid throwing errors inside of helper functions if possible, as that is mostly an anti-pattern. I believe the current Harmonic core already shows this anti-pattern, and I believe it will be hard to fix this without increasing code duplication a bit, so you can leave that for me to clean up later as well.

soapdog commented 9 years ago

I will update this PR and get back to you shortly.

soapdog commented 9 years ago

damn... broke half the tests with the new implementation. This will take a while rsrsrsrs...

UltCombo commented 9 years ago

@soapdog many of the tests cascade, make sure you've removed the findHarmonicRoot from the harmonic init. Apart from that, your logic seems fine.

soapdog commented 9 years ago

PS: Also increased timeout in build.js because it was failing on my machine because my machine is slow. :-P

UltCombo commented 9 years ago

@soapdog Fantastic work!

Your "solving conflicts" merge seemed to have some issues, so I've rebased and squashed your commits into 90bc5390907034e07fadaadd6ced26dfb4e98c10. I've also fixed some styling issues (removed trailing whitespace, collapsed multiple newlines, prefer single quotes over double quotes, etc.)

soapdog commented 9 years ago

@UltCombo thanks a lot for the rework. I did that all sleepy and I think I did something wrong that triggered some conflicts. I will take more care of the styling guide for future work.

UltCombo commented 9 years ago

@soapdog no problem. ;) I've worked a bit on your MissingFileError, now it uses class syntax. I think we can use that error for missing theme files as well.

soapdog commented 9 years ago

@UltCombo thanks a lot for that. My mind tends to be in old JS ways and I keep forgetting to try to use the new stuff.