-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 blueprint #377
Refactor blueprint #377
Conversation
}; | ||
|
||
return blueprint.install(cwd, locals, options.dryRun); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems to be a step in the wrong direction.
I would prefer to have a blueprint object and a fileinfo object that encapsulates this complexity and clearly explains intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't hold any state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it encapsulates complexity, and maintains the blueprint sessions state.
Churn for the sake of churn really causes a lot of un-needed noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What session state? The blueprint is a folder with a bunch of files. I'd really like to keep it that simple.
I rewrote it because I didn't understand the old version. Isn't it much easier to understand now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so.
State includes dependency instances. Entities should not have explicit dependencies on there collaborators, this just becomes unmaintainable. A solution for this is injected a dependency during entity construction. Then these collaborator instances are available at runtime, following this approach continues to decouple our project and also simplify testing.
When fixing something, the smallest amount of refactoring to facilitate the fix is appropriate, or submit a refactoring first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I don't get it. What dependencies are you talking about. Could you explain this in simpler terms? What do you mean by a collaborator?
My concept:
Tell blueprint task the blueprint folder -> prompts user if there are conflicts -> copies files into cwd
I don't care whether we use this version. Main problems with the old one are:
It's output doesn't go through ui all the timeEdit: Patched in ui.prompt with inquirer #411- It's output is verbose even if
--verbose
wasn't specified - It's code is twice as long and complicated as it needs to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkout the command refactor to see what i mean by collaborator. As that is finished I'll update the arch doc accordingly.
I believe the code can be improved, but I also believe you have not noticed all the functionality. Such as: Prompting the user each question first, and later batch applying.
Additionally a Blueprint is a "real thing", it has collaborators, it carries the state of the current session. Additionally, we plan to use this constructor for our generators.
I am very fine with improving the current constructs, but not to disembody them.
#411 now fixes the tests on windows an pimps #377 (comment) pretty much summarizes of what I think of the current blueprint implementation. The flow I describe there:
I want some convincing comments on why moving all blueprint related stuff into the |
I'm closing this. We plan to unify generators with the blueprint. The blueprint we have now would then only become another generator. #351 |
See discussion below.