-
Notifications
You must be signed in to change notification settings - Fork 752
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
base: main
Are you sure you want to change the base?
feat: added support to pom.xml and build.gradle #441
Conversation
This PR is still pending:
|
hi @augustomelo thank you for the PR, we will take a look at it |
4669f97
to
cb6ae6b
Compare
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 |
Hey, regarding the failing workflows, I believe there is something strange with them, because for example the code scanning:
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: Not sure how to proceed now, @panticmilos do you have any ideas here ? |
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? |
Hey @panticmilos thanks for the reply! Sure thing I will have a look on it once I have the time 😄 |
ad6ca9d
to
d502a04
Compare
d502a04
to
6abf828
Compare
hey @panticmilos did a refactor using regex, unfortunately I had to remove a way that the java version is retrieved from the |
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 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 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? |
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:
|
a806815
to
bc66657
Compare
Hey @augustomelo, I made a PR to your fork to add support for versioning via |
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 ? |
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. |
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 |
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.
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 |
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: