Fixed #52 - Incorrect behavior when generate module which already exists #59
Conversation
| @@ -5,88 +6,85 @@ | |||
| module Generamba | |||
|
|
|||
| # Responsible for creating the whole code module using information from the CLI | |||
| class ModuleGenerator | |||
| class ModuleGenerator < Thor | |||
There was a problem hiding this comment.
Why are you subclassing this class from Thor?
There was a problem hiding this comment.
If I don't subclassing from Thor, I don't use yes? method, from Thor module. Correct me if I am wrong.
There was a problem hiding this comment.
Oh, I missed two lines with the yes method.
I don't like the idea of using Thor and its functions on the business logic layer (and ModuleGenerator) definitely belongs to this layer. We shouldn't mess presentation and business logic in one class.
I see two possible solutions:
- You can perform this check somewhere in the
gen_command.rb(maybe by using some helper class) - You can post a warning, that these files are not empty, and stop the module generation process. To force overwrite, you can add some flag to
generamba gencommand, likegeneramba gen MyModule MyTemplate --force.
What do you think?
There was a problem hiding this comment.
I think, perform this check in gen_command.rb right idea. And I noticed, that I don't sold this check, but only sold yes answer :( Then, I'll implementing this method in xcodeproj_helper.rb and move this check in gen_command.rb.
|
@Beniamiiin thanks for contributing, a really nice implementation! I have a couple of questions - you can see them in code review. Maybe you want to implement also another part of the issue? |
|
Yes, I am implementing this too. |
| project = XcodeprojHelper.obtain_project(code_module.xcodeproj_path) | ||
| module_group_already_exists = XcodeprojHelper.module_with_group_path_already_exists(project, code_module.module_group_path) | ||
|
|
||
| if (module_group_already_exists == true) |
There was a problem hiding this comment.
As far as I know, ruby style guidelines require not using brackets for if cases
There was a problem hiding this comment.
I use brackets only this module, because in this module already used brackets.
|
@Beniamiiin just a couple of minor fixes - and we can merge it! I thought of releasing a new version today - but I'll be unavailable for the following 10 days, so I won't be able to make a hotfix if something goes wrong. I think we can schedule a new version something near the 15th of January. |
|
Thanks once again! |
Fixed #52 - Incorrect behavior when generate module which already exists
No description provided.