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

feat: added support to pom.xml and build.gradle #441

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

augustomelo
Copy link

@augustomelo augustomelo commented Jan 8, 2023

Description:
The idea of this PR is to extend the java-version-file to support pom.xml, because for now it only supports the java-version-file created by jEnv

Related issue:
Add link to the related issue.

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@augustomelo
Copy link
Author

augustomelo commented Jan 8, 2023

This PR is still pending:

  • Add unit e2e test to cover the other scenarios
  • Add documentation

@panticmilos
Copy link
Contributor

hi @augustomelo thank you for the PR, we will take a look at it

@augustomelo augustomelo marked this pull request as ready for review January 15, 2023 16:35
@augustomelo augustomelo requested a review from a team as a code owner January 15, 2023 16:35
@augustomelo augustomelo force-pushed the feature/add-support-for-pom.xml branch from 4669f97 to cb6ae6b Compare January 16, 2023 08:56
@augustomelo
Copy link
Author

Hello @panticmilos could you approve the workflow run ? I tested it locally with act and it was fine, just wanted to sure that I didn't break anything

@augustomelo
Copy link
Author

Hey, regarding the failing workflows, I believe there is something strange with them, because for example the code scanning:

  • GitHub Code Scanning / CodeQL complains in a file that is generated dist/cleanup/index.js
    • 97022: if (util_1.isObject(p1) || (util_1.isString(p1) && (/^\s*</.test(p1) || /^\s*[\{\[]/.test(p1) || /^(\s*|(#.*)|(%.*))*---/.test(p1)))) {
    • 97795: else if (p1 !== null && /^(\s*|(#.*)|(%.*))*---/.test(p1))

I believe this is coming from the xmlbuilder2 when I create the object from the string (here), should I change the library ? Because I don't think I have any control over it.

The other workflows that failed are:

Which are failing during the install dependencies step, it could be because the dependency that I added: xpath which has the same license as the one xmlbuilder2 (MIT).

Not sure how to proceed now, @panticmilos do you have any ideas here ?

@panticmilos
Copy link
Contributor

hi @augustomelo, and sorry for the delayed response. We had some chats about this PR, and we came to the conclusion that the 'xpath' is probably the one that is causing the issues. Now, we were wondering could the same goal be achieved without 'xpath'?

Also can you please rebase to the main branch?

@augustomelo
Copy link
Author

Hey @panticmilos thanks for the reply! Sure thing I will have a look on it once I have the time 😄

@augustomelo augustomelo force-pushed the feature/add-support-for-pom.xml branch 2 times, most recently from ad6ca9d to d502a04 Compare April 8, 2023 12:48
@augustomelo augustomelo force-pushed the feature/add-support-for-pom.xml branch from d502a04 to 6abf828 Compare April 8, 2023 12:49
@augustomelo
Copy link
Author

hey @panticmilos did a refactor using regex, unfortunately I had to remove a way that the java version is retrieved from the maven-compiler-plugin, because I don't think it will reliably identify the java version

@IvanZosimov IvanZosimov added feature request New feature or request to improve the current logic needs eyes labels Jul 7, 2023
@tim-we
Copy link

tim-we commented Mar 3, 2024

Hey @augustomelo, first of all thanks for the PR, if this didn't exist I would have created my own ;)

But I wonder why you chose to parse the version via regex as that approach obviously limited. This project already has xmlbuilder2 as a dependency which can also be used for parsing XML. For example we could do this:

const doc = create(pomContentAsAString); // create function from xmlbuilder2
const mvnVersionNode = doc.find(n => n.node.nodeName === "maven.compiler.source", false, true);
const sourceVersion = mvnVersionNode.first().toString();

This way we can have a more robust implementation and support using the version specified in maven-compiler-plugin.

Are you still willing to work on this @augustomelo? Otherwise I can create a new PR.

@panticmilos has this feature request been discussed? Are you willing to merge something like this?

@augustomelo
Copy link
Author

augustomelo commented Mar 4, 2024

hey @tim-we, thanks for the review and I much appreciate the suggestion the solution is much better now!

I am not sure why, but when I read the xmlbuilder2 I understood that it was not possible to create the xml object from a string, therefore I used the xpath and then moved to using regex.

@panticmilos when you give a green light on this I will fix the merge conflicts

edit:

  • added last paragraph

@augustomelo augustomelo force-pushed the feature/add-support-for-pom.xml branch from a806815 to bc66657 Compare March 4, 2024 08:30
@tim-we
Copy link

tim-we commented Apr 13, 2024

Hey @augustomelo, I made a PR to your fork to add support for versioning via maven-compiler-plugin. Please have a look. There are also some minor things we should address before this gets merged, such as variable names not being accurate anymore after the refactor. If this progresses any further I'd might do another code review.

src/util.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
@augustomelo
Copy link
Author

hey @tim-we I didn't forgot about this, when I had the time I will apply the changes. I was thinking about also add support to gradle, but I currently don't know how to achieve that besides using RegEx, do you have any ideas ?

@tim-we
Copy link

tim-we commented Apr 25, 2024

No I actually don't know that much about gradle (currently learning). But I'm not aware of any parsers if thats what you mean.

@augustomelo
Copy link
Author

hey @tim-we refactored the code to align with your comments, feel free to resolve the conversation.

Added the support to builld. gradle using regex.

I didn't do the merge, because I am not sure if this PR is going to be merged

@augustomelo augustomelo changed the title feat: added support to pom.xml feat: added support to pom.xml and build.gradle Apr 27, 2024
When trying to find a node the function wasn't iterating over the whole XML tree, which made the function not able to
correctly identify the java version

The regex is declared as string, which made it necessary to add more escaping.
@augustomelo
Copy link
Author

hey @tim-we I had to do some changes on the code since the testes were failing in correctly identifying the java version, now they seem fine, the failure is now not being able to find the version to download

@panticmilos and @IvanZosimov any new if this is going to be merged? if not I will not bother rebasing it for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic needs eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants