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

[FIX] ApplicationFormatter: detect the namespace from component controller #243

Merged
merged 2 commits into from
Apr 24, 2019

Conversation

petermuessig
Copy link
Contributor

@petermuessig petermuessig commented Apr 21, 2019

When the manifest.json contains a Maven placeholder the namespace cannot
be detected properly. As a fallback the namespace will now be derived
from the component controller.

Thank you for your contribution! 🙌

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 84.974% when pulling 04e8ddb on fix-appnamespace into 87fd85e on master.

@coveralls
Copy link

coveralls commented Apr 21, 2019

Coverage Status

Coverage increased (+0.1%) to 85.28% when pulling bca6d0b on fix-appnamespace into df51ca0 on master.

@petermuessig
Copy link
Contributor Author

Maybe the fix is not the most optimal one, since it doesn’t use AST parsing to find the Component namespace and a regex instead but this definitely works (and is IMO as reliable as the AST one compared to the code and runtime effort being necessary to do AST parsing)!

@petermuessig
Copy link
Contributor Author

Enhanced the commit to read the pom.xml and be able to replace the Maven placeholders with the dedicated entries or properties from the pom.xml.

lib/types/application/ApplicationFormatter.js Outdated Show resolved Hide resolved
lib/types/application/ApplicationFormatter.js Outdated Show resolved Hide resolved
lib/types/application/ApplicationFormatter.js Show resolved Hide resolved
lib/types/application/ApplicationFormatter.js Outdated Show resolved Hide resolved
lib/types/application/ApplicationFormatter.js Outdated Show resolved Hide resolved
@petermuessig petermuessig requested a review from RandomByte April 23, 2019 18:22
@petermuessig
Copy link
Contributor Author

This time I created a follow up commit which will be squashed before the merge later on...

@petermuessig petermuessig force-pushed the fix-appnamespace branch 2 times, most recently from a30350b to 79a0499 Compare April 23, 2019 18:48
@petermuessig
Copy link
Contributor Author

Hmm, I am wondering why the build is failing! The JSDoc build works locally but somehow centrally it doesn't! I now squashed the change locally manually as this maybe affects the build... :-/

When the manifest.json contains a Maven placeholder the namespace cannot
be detected properly. With this extension the namespace can be resolved
from the pom.xml from the pom entries or Maven properties.
@RandomByte RandomByte dismissed their stale review April 24, 2019 09:20

All resolved

@RandomByte
Copy link
Member

Looks pretty good 👍

@tommyvinhlam to approve.

devtomtom
devtomtom previously approved these changes Apr 24, 2019
Copy link
Member

@devtomtom devtomtom left a comment

Choose a reason for hiding this comment

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

👍

@devtomtom
Copy link
Member

Rebase required

Copy link
Member

@devtomtom devtomtom left a comment

Choose a reason for hiding this comment

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

Looks good!

@devtomtom devtomtom merged commit 49ecb07 into master Apr 24, 2019
@RandomByte RandomByte deleted the fix-appnamespace branch April 24, 2019 12:32
devtomtom pushed a commit that referenced this pull request Jun 17, 2019
…rs (#243)

When the manifest.json contains a Maven placeholder the namespace cannot
be detected properly. With this extension the namespace can be resolved
from the pom.xml from the pom entries or Maven properties.
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.

5 participants