Skip to content

#Issue-142: Action/Code Generation. New CLI command Generation #240

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

Merged
merged 14 commits into from
May 5, 2020
Merged

#Issue-142: Action/Code Generation. New CLI command Generation #240

merged 14 commits into from
May 5, 2020

Conversation

coderimus
Copy link
Contributor

@coderimus coderimus commented Apr 29, 2020

Description (*)
This PR provides a new action: Magento 2 CLI Command
image

This new action has the next form
image

All fields are required. The Console is a predefined value

For example, the next data set
image

The result of its generation will be

CLI PHP CLass
image

di.xml settings
image

If a developer decided to add one more CLI command in the scope of the current module the result will be next:
image

Fixed Issues (if relevant)
#142

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with integration/functional tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@coderimus coderimus changed the title 142/cli command generation [WIP] 142: Action/Code Generation. New CLI command Generation Apr 29, 2020
Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @coderimus, really great job so far 👍. I've left couple of notices, please check them as well.
Thank you.

</constraints>
<properties>
<labelFor value="61353"/>
<text resource-bundle="magento2/common" key="common.cli.class.name"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use common.className instead of adding a new one. Please adjust everywhere where it is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eduard13 thank you for the review! Got your point and totally agree with it 👍 Will apply changes with the next push.

@@ -27,3 +27,10 @@ common.notAvailable=N/A
common.classInheritance=Inherit Class
common.license.proprietary=Proprietary
common.description=Description
common.cli.class.name=Class Name
common.cli.parent.directory=Parent Directory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use it like this common.parentDirectory as it may reused in some other components.

</constraints>
<properties>
<labelFor value="d786e"/>
<text resource-bundle="magento2/common" key="common.cli.cli.description"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have common.description.

Suggested change
<text resource-bundle="magento2/common" key="common.cli.cli.description"/>
<text resource-bundle="magento2/common" key="common.description"/>

</constraints>
<properties>
<labelFor value="a06d9"/>
<text resource-bundle="magento2/common" key="common.cli.cli.name"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Suggested change
<text resource-bundle="magento2/common" key="common.cli.cli.name"/>
<text resource-bundle="magento2/common" key="common.name"/>

</constraints>
<properties>
<labelFor value="82342"/>
<text resource-bundle="magento2/common" key="common.cli.parent.directory"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<text resource-bundle="magento2/common" key="common.cli.parent.directory"/>
<text resource-bundle="magento2/common" key="common.parentDirectory"/>

Comment on lines 46 to 47
"validator.file.cantBeCreated",
commonBundle.message("common.cli.class.title")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have some indents here

Suggested change
"validator.file.cantBeCreated",
commonBundle.message("common.cli.class.title")
"validator.file.cantBeCreated",
commonBundle.message("common.cli.class.title")

protected CommonBundle bundle;

public AbstractDialog() {
this.bundle = new CommonBundle();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point 👍

@@ -13,22 +13,27 @@
import com.magento.idea.magento2plugin.magento.packages.Package;
import com.magento.idea.magento2plugin.util.CamelCaseToSnakeCase;

@SuppressWarnings({
"PMD.FieldNamingConventions",
"PMD.LongVariable",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's suppress the LongVariable on a global level. I believe variable names should be descriptive and self-explanatory.

@coderimus coderimus changed the title [WIP] 142: Action/Code Generation. New CLI command Generation #Issue-142: Action/Code Generation. New CLI command Generation May 4, 2020
@VitaliyBoyko VitaliyBoyko removed their assignment May 4, 2020
@VitaliyBoyko VitaliyBoyko requested a review from eduard13 May 4, 2020 14:58
@coderimus
Copy link
Contributor Author

@eduard13 requested change added and the work completed in the scope of the PR issue. Please, review this PR 😃

Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @coderimus it works really great ✅. Could you please check on minor note and share your thought regarding that one?
Also looks like something is wrong with the dialog elements, probably with JPanels. Check the following screenshot (but it's also visible in PR description)
image
Let's make it to looks like this:
image

@@ -27,3 +27,10 @@ common.notAvailable=N/A
common.classInheritance=Inherit Class
common.license.proprietary=Proprietary
common.description=Description
common.parentDirectory=Parent Directory
common.cliCommandName=CLI Command Name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think if we'll keep it just Command Name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, will change :)

@coderimus
Copy link
Contributor Author

@eduard13 thank you for your review and suggestions 👍 I implemented them. Please, review.
image

Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome.
Good job 💯

@VitaliyBoyko VitaliyBoyko merged commit 06c1505 into magento:1.0.1-develop May 5, 2020
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.

4 participants