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

Split functionality in lib and CLI #40

Merged
merged 60 commits into from
May 6, 2019
Merged

Split functionality in lib and CLI #40

merged 60 commits into from
May 6, 2019

Conversation

fnogatz
Copy link
Owner

@fnogatz fnogatz commented Aug 5, 2018

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 fnogatz added this to the 2.0 milestone Aug 5, 2018
@fnogatz fnogatz self-assigned this Aug 5, 2018
@fnogatz
Copy link
Owner Author

fnogatz commented Feb 26, 2019

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.

@fnogatz fnogatz changed the title [WIP] Rewrite to split functionality in lib and CLI Split functionality in lib and CLI Feb 27, 2019
@BeniRupp
Copy link
Collaborator

BeniRupp commented Mar 5, 2019

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

@fnogatz
Copy link
Owner Author

fnogatz commented Mar 27, 2019

@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? 😸

@BeniRupp
Copy link
Collaborator

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

@fnogatz
Copy link
Owner Author

fnogatz commented Apr 25, 2019

@BeniRupp, ping 😀

@BeniRupp
Copy link
Collaborator

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
Copy link
Collaborator

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:

  • move all the command functions in bin/index.js into own modules (maybe: basic, export, report, archive?) so that this file does "only" add all the commands to the commander instance.
  • create a test file for each command to split up the very long test/index.js

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! 😉

lib/index.js Show resolved Hide resolved
bin/index.js Outdated Show resolved Hide resolved
bin/index.js Outdated Show resolved Hide resolved
bin/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
@BeniRupp
Copy link
Collaborator

BeniRupp commented May 6, 2019

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
Copy link
Owner Author

fnogatz commented May 6, 2019

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 😆

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
Copy link
Collaborator

BeniRupp commented May 6, 2019

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

@BeniRupp BeniRupp merged commit 3c5a776 into master May 6, 2019
@fnogatz fnogatz deleted the lib branch May 6, 2019 21:05
@fnogatz
Copy link
Owner Author

fnogatz commented May 6, 2019

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
Copy link
Collaborator

BeniRupp commented May 7, 2019

Yes, he gave it to me. 🙂

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

@fnogatz
Copy link
Owner Author

fnogatz commented May 8, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants