-
Notifications
You must be signed in to change notification settings - Fork 36
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
Rearrange src/commands directory following the new conventions #165
Conversation
Question: Should we add a convention that requires a verb for the action? The drawback I see is that the command names will be longer. Unless we apply this convention for class names only, but we would have a problem of consistency. |
I dont test all commands but a lot it's ok for me ! Can you rebase the PR for the conflicts |
Converted to draft. There is (at least) 2 different ways to switch the command names :
|
It would be hard work but I can do it progressively. Are you referring to this documentation? |
I don't think it will be hard. |
Not so hard, but quite long. I'll have to retype every old command class name, name and service name to mark them as deprecated if I understand well. I'm gonna study it after validator step. |
/!\ Need your opinion /!\@SebSept @okom3pom @jf-viguier @nenes25 I moved the validator in a |
Too general names are subject to become a mess. fyi, I worked on this subject with @Kaudaj , and this validator will be used via a phpstan rule. |
Notice, you can have a "dev-autoload" section in composer.json |
Looks like PrestaShop put Validation classes in src folder, same for PrestaShop modules. |
the src folder is currently perfectly clean, adding the validator doesn't look good to me. |
The most relevant documentation I found is about Validator component. Or maybe I missunderstood, what do you mean exactly by testing tools? |
The Validator in a class for the main Symfony code, not for testing Symfony, so it's normal to find it in src. Here https://github.com/symfony/symfony/tree/5.4/src/Symfony/Component/Dotenv we can see a folder Tests, it looks like the tests folder are at the same of the components, in a src folder. (it will be src/Tests/Validator for us, because it's a generic test) Having a test folder at the level looks good but is not the PrestaShop way. So it's possible to :
... |
Ok so if I follow you, we put it in |
Yep, looks good to me. |
@SebSept yes i agree with your point of view. |
I let others do the job as I'm on holiday 😎 |
The status is draft ? |
Command name's will change, right ? |
What do you mean exactly? |
'fop:debug-mode' is not available anymore. It's replaced by 'fop: environment:debug-mode' |
Ah yea ok, I forgot that! Thought you wanted to change command names again. |
I guess, there is no need to set alias for services, this is not really services but just commands declarations. |
The next step will be to emit deprecation warnings but that could be done in another PR. |
Do you mean in the code with PHPDoc deprecated keyword? I don't really see how to warn user in console output... |
I mean in console output. |
@SebSept I have added a deprecated warning, please check when you can. |
Good 👍 |
I updated the issue, not a draft anymore, since there is no breaking change. |
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.
Great job !
I can see backward compatibility is ok.
You've made some tests 🎉 👍🏼
I've just tested a few commands, look good to me :)
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.
Good Job !
It's fine to me too 🎆
I've discovered when testing that if you use the command bin/console list fop
alias are listed too. 😄
Rearrange src/commands directory following the new conventions defined in issue #85.