-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Refactor index.js #1496
Comments
My main concern with splitting up the monolithic A minor downside is refactoring the file makes it a harder to track back through the history to find why certain lines of code and logic were introduced. But the benefits to future work outweigh this. For interest, the file was split up in #661 in a branch to modernise the codebase for v3. |
About the first concern- I am not sure I can't think of a use case where client could be broken because of such an internal change, as long as the api stays the same. The api is the contract, everything else can be changed. If someone is doing some custom scripting based on anything else other than api, it's not About the downside- yep, this is an annoying limitation of git, but as you said, the benefits to future work outweigh the this. For me it was super annoying to orient myself inside this huge index.js file, and a split is a no-brainer decision. |
I am having a play around with refactoring. Naively splitting Help and Command introduced a circular dependency, but @cravler supplied a work-around with subclass to break the loop. One class per file. https://github.com/tj/commander.js/tree/feature/refactor-experiment As of 17 Apr the extracted |
I'll have another go at this when Pull Requests are quiet. |
few notes:
|
Released in Commander v8. |
Are we intentionally keeping this god
index.js
file as is, or are there plans for breaking it down to more manageable pieces?I am interested in contributing more stuff to this lib, and I thought that breaking up this huge file could be a good start if there are no objections...
The text was updated successfully, but these errors were encountered: