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

Renamed local variable in maven archetype quickstart #26

Conversation

judovana
Copy link
Contributor

@judovana judovana commented Dec 25, 2023

There is macro which is using parameter and its local occurrence of same name as is global property, thus shadowing it. This PR is renaming it so it is unique or at least distinguishable (second commit)

  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@@ -1,11 +1,11 @@
<?xml version="1.0" encoding="UTF-8"?>
#macro( compilerProperties $javaCompilerVersion )
#macro( compilerProperties $javaCompilerVersionLocal )
Copy link

Choose a reason for hiding this comment

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

Is this variable set by user code/properties somewhere that will need to update to the new name?

Copy link
Contributor Author

@judovana judovana Jan 3, 2024

Choose a reason for hiding this comment

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

no. TL/DR, yes and no,which was the reasoning of the PR. javaCompilerVersion is a property which can (and should) be used to set desired bytecode version, however in maven-archetype-quickstart it is partially broken (https://issues.apache.org/jira/projects/MARCHETYPES/issues/MARCHETYPES-77) .
In the line of question, however it is not the variable, but it's macro-local copy. See the calls:


and

When I was trying to fix the https://issues.apache.org/jira/projects/MARCHETYPES/issues/MARCHETYPES-77, this local occurrence was still meddling in.

Note, that this PR suggest two names. I prefer the one ion first commit, but the second is probably better. I'm ok with any name you would be happy with. TY!

compilerProperties had one parameter - javaCompilerVersion. This commit
is renaming this parameter and its usage to jcv, so it is no longer
exchangable for the property
@judovana judovana force-pushed the renamedLocalVariableIn-maven-archetype-quickstart branch from bb92797 to a1e637a Compare January 3, 2024 08:16
@judovana
Copy link
Contributor Author

judovana commented Jan 3, 2024

rebased to master at a0f0e9a , to see ti against all jdk8-21 tests.

@elharo elharo merged commit e0da200 into apache:master Jan 3, 2024
13 checks passed
@judovana
Copy link
Contributor Author

judovana commented Jan 3, 2024

tyvm sir!

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