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

added parser support for EL 3.0: += ; = -> #734

Merged
merged 3 commits into from May 19, 2022

Conversation

thiesw
Copy link
Contributor

@thiesw thiesw commented May 8, 2022

  • added unit tests
  • improved output of parser unit test
  • NOTE: no semantic and variable definition/resolving has been added
    (yet)

Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Please resubmit with the Sign-Off header for the PR to be accepted

@thiesw
Copy link
Contributor Author

thiesw commented May 9, 2022

Is the sign-off in the last commit sufficient? Or what to do otherwise?

@jeffmaury
Copy link
Member

Is the sign-off in the last commit sufficient? Or what to do otherwise?

No Sign Off should be on every commit so you must rewrite history and force push then

Thies Wellpott added 2 commits May 9, 2022 11:04
- added unit tests
- improved output of parser unit test
- NOTE: no semantic and variable definition/resolving has been added
(yet)

Signed-off-by: Thies Wellpott <twforen@online.de>
Signed-off-by: Thies Wellpott <twforen@online.de>
@thiesw
Copy link
Contributor Author

thiesw commented May 9, 2022

Build has error "compare-version-with-baselines". That message is true but - in my opinion - changing plugin version is out of my scope for a (small) change. Maintainer of the plugin will do after incorporating all desired changes. Or am I wrong here?

@jeffmaury
Copy link
Member

Build has error "compare-version-with-baselines". That message is true but - in my opinion - changing plugin version is out of my scope for a (small) change. Maintainer of the plugin will do after incorporating all desired changes. Or am I wrong here?

Yes because we need the unit tests to run before accepting a PR. I will handle the version bump

Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@thiesw
Copy link
Contributor Author

thiesw commented May 10, 2022

Did you perform the version bump manually in all the files (maybe search&replace) or is there a "bump version" command inside maven-pom?
Other question: When trying to build locally (maven package inside directory \jbosstools-base\common\plugins\org.jboss.tools.common.el.core) I get the error
Missing requirement: org.jboss.tools.common.el.core 3.17.200.qualifier requires 'osgi.bundle; org.jboss.tools.common.resref.core 0.0.0' but it could not be found
Do I have to globally add another maven repository?

@jeffmaury
Copy link
Member

Did you perform the version bump manually in all the files (maybe search&replace) or is there a "bump version" command inside maven-pom? Other question: When trying to build locally (maven package inside directory \jbosstools-base\common\plugins\org.jboss.tools.common.el.core) I get the error Missing requirement: org.jboss.tools.common.el.core 3.17.200.qualifier requires 'osgi.bundle; org.jboss.tools.common.resref.core 0.0.0' but it could not be found Do I have to globally add another maven repository?

No I run a tycho command from the common sub folder.
The error is normal as base is to be build from the root or you need to run mvn install if you want to run on a single plugin

@thiesw
Copy link
Contributor Author

thiesw commented May 10, 2022

Ok. maven verify inside \jbosstools-base works. But I get the same compare-version-with-baselines error (as expected).
How can I change the version number myself to get a successful build? Maybe there is already a documentation for this?
Shall I include all the version bumped pom.xml in my pull request - would be required for the automated build to succeed. But this is in my opinion a bit large for a change inside a sub-sub-project. Or is it possible to skip just this compare-version-with-baselines for a local build? And what to include in the pull request then (because best would be to have a successful build for a submitted PR) ?
Maybe this info can/should be included in the README.md?

@jeffmaury
Copy link
Member

Ok. maven verify inside \jbosstools-base works. But I get the same compare-version-with-baselines error (as expected). How can I change the version number myself to get a successful build? Maybe there is already a documentation for this? Shall I include all the version bumped pom.xml in my pull request - would be required for the automated build to succeed. But this is in my opinion a bit large for a change inside a sub-sub-project. Or is it possible to skip just this compare-version-with-baselines for a local build? And what to include in the pull request then (because best would be to have a successful build for a submitted PR) ? Maybe this info can/should be included in the README.md?

See https://github.com/jbosstools/jbosstools-devdoc/blob/main/source/build_checks.adoc#compare-version-with-baselines

@thiesw thiesw requested a review from jeffmaury May 10, 2022 16:15
Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM

@jeffmaury jeffmaury merged commit 123d56e into jbosstools:main May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants