search

fnogatz / clocker

Command-line tool to track project hours
Other
420 stars 31 forks source link

Split functionality in lib and CLI #40

Closed fnogatz closed 5 years ago

fnogatz commented 6 years ago

As first discussed 3 years ago in #26, I just started to move the core functionality into a /lib/index.js file, so we can split into a library and the CLI. This also allows to provide TAP tests for future development.

fnogatz commented 5 years ago

I finished re-implementing all of clocker's current CLI methods. @substack, @BeniRupp and other users, would you please give the new version of the lib branch a try? Just use ./bin/index.js instead of ./bin/cmd.js.

BeniRupp commented 5 years ago

@fnogatz I will have a look at your changes as soon as I have a free timeslot for that. 👍

fnogatz commented 5 years ago

@fnogatz I will have a look at your changes as soon as I have a free timeslot for that. +1

@BeniRupp Already had a chance to look into the changes? :smile_cat:

BeniRupp commented 5 years ago

Yes, but only a quick look. Sorry for that. I hope that I can give you some detailed feedback next week. 🙈

fnogatz commented 5 years ago

@BeniRupp, ping :grinning:

BeniRupp commented 5 years ago

I'am so sorry! My smartphone reminds me every other day.. 😞 I don't promise anything, but I have it in mind and I want to give you some feedback.

BeniRupp commented 5 years ago

Soo.. I had the change to have a look at your PR this evening. 🙂

First of all, I like the refactoring you made! To use commander instead of the else-if hell is a huge benefit in my opinion. Separate functions for the commands are great, too. 👍 And tests are always a good choice! 👏 So you added a lot of improvements to the code. Thanks a lot for that!

One thing that I would reconsider is the length of the files. I prefer short files for a better overview and to separate things that belong together from those which do not.

So my suggestion is:

I think this would really improve the readability.

I will also add some comments to the code to point out some smaller things I found during the review.

Hopefully this feedback is useful for you. If I am wrong with one of my comments, please let me know! 😉

BeniRupp commented 5 years ago

Hey @fnogatz, thank you for the rework! 🙂 So I think we are ready for merging the feature branch, right? Or do you wait for some other feedback?

fnogatz commented 5 years ago

There's still one major point from your original post:

One thing that I would reconsider is the length of the files. I prefer short files for a better overview and to separate things that belong together from those which do not.

I see your point in splitting the large files. However, my urge to refactor this is not too big :laughing:

But I am open to your suggestions. Maybe it's a good idea to merge this first, and then you could open a PR with the changes, if you are still interested in? I will happily merge it :)

BeniRupp commented 5 years ago

Sounds good! 👍 Let's do it this way.

fnogatz commented 5 years ago

Haha, thanks for merging this. I once asked @substack to give you the permissions for this repository, but he never responded. So you are also able to publish the new version at npm?

BeniRupp commented 5 years ago

Yes, he gave it to me. 🙂

Oh, I've never done this. Is it just a npm publish in the repo root?

fnogatz commented 5 years ago

I just removed the legacy bin/cmd.js and bin/usage.txt and published the new version as v1.15.0 on npm. This is no major semver update, because it should contain no breaking changes, only a changing implementation.