Skip to content
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

O3-3776: Add fix for content package archetype #294

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

mherman22
Copy link
Member

@mherman22 mherman22 commented Sep 25, 2024

Description of what I changed

The feedback from those who have tried creating a content package archetype has led to the following adjustments:

  • Fixed the issue with modulename where a user ended up with {moduleName}name> in the POM instead of the real name that has been set.
  • This message below was tweaked to default to org.openmrs.content and group the language about "modules".
By convention OpenMRS modules use 'org.openmrs.module' as a groupId 
(must follow convention for naming java packages) and the module id 
as an artifactId. The version should follow maven versioning convention, 
which in short is: major.minor.maintenance(-SNAPSHOT).

Please specify groupId (default: 'org.openmrs.module'):
  • I removed the prompmt What version of the content package (-Dcontentpackage) do you want to support? (default: '1.0.0'):

Issue I worked on

see https://openmrs.atlassian.net/browse/SDK-

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • I ran mvn clean install right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute the above command

  • All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Thanks @mherman22! Mostly looks good. A couple of small suggestions for simplifying thing.

Comment on lines 265 to 284
if (TYPE_CONTENT_PACKAGE.equals(type)) {
wizard.showMessage(CONTENT_MAVEN_INFO);
groupId = wizard.promptForValueIfMissingWithDefault(GROUP_ID_PROMPT_TMPL, groupId, "groupId", "org.openmrs.content");
while (!groupId.matches("[a-z][a-z0-9.]*")) {
wizard.showError("The specified groupId " + groupId
+ " is not valid. It must start from a letter and can contain only alphanumerics and dots.");
groupId = null;
groupId = wizard
.promptForValueIfMissingWithDefault(GROUP_ID_PROMPT_TMPL, groupId, "groupId", "org.openmrs.content");
}
} else {
wizard.showMessage(MAVEN_INFO);
groupId = wizard.promptForValueIfMissingWithDefault(GROUP_ID_PROMPT_TMPL, groupId, "groupId", "org.openmrs.module");
while (!groupId.matches("[a-z][a-z0-9.]*")) {
wizard.showError("The specified groupId " + groupId
+ " is not valid. It must start from a letter and can contain only alphanumerics and dots.");
groupId = null;
groupId = wizard
.promptForValueIfMissingWithDefault(GROUP_ID_PROMPT_TMPL, groupId, "groupId", "org.openmrs.module");
}
Copy link
Member

Choose a reason for hiding this comment

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

Can't this be simplified?

Suggested change
if (TYPE_CONTENT_PACKAGE.equals(type)) {
wizard.showMessage(CONTENT_MAVEN_INFO);
groupId = wizard.promptForValueIfMissingWithDefault(GROUP_ID_PROMPT_TMPL, groupId, "groupId", "org.openmrs.content");
while (!groupId.matches("[a-z][a-z0-9.]*")) {
wizard.showError("The specified groupId " + groupId
+ " is not valid. It must start from a letter and can contain only alphanumerics and dots.");
groupId = null;
groupId = wizard
.promptForValueIfMissingWithDefault(GROUP_ID_PROMPT_TMPL, groupId, "groupId", "org.openmrs.content");
}
} else {
wizard.showMessage(MAVEN_INFO);
groupId = wizard.promptForValueIfMissingWithDefault(GROUP_ID_PROMPT_TMPL, groupId, "groupId", "org.openmrs.module");
while (!groupId.matches("[a-z][a-z0-9.]*")) {
wizard.showError("The specified groupId " + groupId
+ " is not valid. It must start from a letter and can contain only alphanumerics and dots.");
groupId = null;
groupId = wizard
.promptForValueIfMissingWithDefault(GROUP_ID_PROMPT_TMPL, groupId, "groupId", "org.openmrs.module");
}
String defaultGroupId;
if (TYPE_CONTENT_PACKAGE.equals(type)) {
defaultGroupId = "org.openmrs.content";
wizard.showMessage(CONTENT_MAVEN_INFO);
} else {
defaultGroupId = "org.openmrs.module";
wizard.showMessage(CONTENT_MAVEN_INFO);
}
// code can be shared here

Comment on lines 291 to 309
packageName = "org.openmrs.module." + artifactId;
packageName = TYPE_CONTENT_PACKAGE.equals(type) ? "org.openmrs.content." + artifactId : "org.openmrs.module." + artifactId;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we refactor this into a function call or using the above:

packageName = groupId + "." + artifactId

That said, we shouldn't need to prompt for a package for a content package since it shouldn't have code.

@ibacher ibacher merged commit 9782465 into openmrs:master Sep 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants