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.

Sign up for GitHub

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

Jump to bottom

Add -Aversion command-line option; fixes #3381 #3464

Merged
merged 14 commits into from Jul 20, 2020
Merged

Add -Aversion command-line option; fixes #3381 #3464

merged 14 commits into from Jul 20, 2020

Conversation

PRITI1999
Copy link
Contributor

#3381 Add -Aversion option

@mernst mernst changed the title Fix issue #3381 Add -Aversion command-line option; fixes #3381 Jul 11, 2020
mernst
mernst requested changes Jul 11, 2020
View reviewed changes
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated Show resolved Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated Show resolved Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated Show resolved Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated Show resolved Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated Show resolved Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated Show resolved Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated
if (hasOption("version")) {
String version = getCheckerVersion();
if (version == null) {
messager.printMessage(Kind.NOTE, "Version info not available!");
Copy link
Member

Choose a reason for hiding this comment

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

Under what circumstances is version null? Is that a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its never null for the current version. It can only be null if the tag in release.properties is not found in XML_FILE. Should I remove this check altogether and throw an exception if ever version is null?

Copy link
Member

Choose a reason for hiding this comment

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

Since that would indicate an unexpected problem, I think that is the best approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes accordingly. Please re-review.

Copy link
Member

Choose a reason for hiding this comment

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

@PRITI1999 CI does not pass. Please make it pass before requesting a review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I apologize. CI succeeds now.

@wmdietl wmdietl linked an issue Jul 13, 2020 that may be closed by this pull request
-version produces "javac (version info not available)" #3381
Closed
mernst
mernst requested changes Jul 14, 2020
View reviewed changes
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated
@@ -2530,4 +2535,33 @@ private void printGitProperties() {
System.out.println("IOException while reading git.properties: " + e.getMessage());
}
}

/**
* Extract the version of the Checker Framework
Copy link
Member

Choose a reason for hiding this comment

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

Minor: end sentences with punctuation (in this case, a period).

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated
* docs/examples/MavenExample/pom.xml file
* @throws NullPointerException if tag present in release.properties for Checker Framework
* version is not found in docs/examples/MavenExample/pom.xml
* @return Checker Framework version {@link String}
Copy link
Member

Choose a reason for hiding this comment

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

It is usually not necessary to state the type, which is already stated nearby in the source code and in the generated API documentation.

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated
* @return Checker Framework version {@link String}
*/
private String getCheckerVersion() throws IOException {
String version = null;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this variable needed?

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated
@@ -2530,4 +2535,33 @@ private void printGitProperties() {
System.out.println("IOException while reading git.properties: " + e.getMessage());
}
}

/**
* Extract the version of the Checker Framework
Copy link
Member

Choose a reason for hiding this comment

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

If you use "Extract", say from where. Even better, I would leave implementation details like that out of the specification and just use "Returns".

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated
return version;
}
}
throw new NullPointerException(
Copy link
Member

Choose a reason for hiding this comment

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

This is an odd choice of exception. Why not use BugInCF?

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated
String line = null;
while ((line = reader.readLine()) != null) {
if (line.split(startTag).length > 1) {
version = line.split(startTag)[1].split(endTag)[0];
Copy link
Member

Choose a reason for hiding this comment

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

This line can be combined with the following one.

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Show resolved Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java Outdated
Properties releaseProperties = getProperties(getClass(), RLS_FILE);
String startTag = releaseProperties.getProperty("checkers.ver.0");
String endTag = releaseProperties.getProperty("checkers.ver.1");
String XML_FILE = "/docs/examples/MavenExample/pom.xml";
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? It crashed for me when I tried to run it, because neither of the two files is not available in checker.jar.

Copy link
Member

Choose a reason for hiding this comment

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

You could use git.properties instead. See method printGitProperties.

@PRITI1999
Copy link
Contributor Author

@mernst Added the suggested changes. Please re-review.

@mernst mernst merged commit 3498970 into typetools:master Jul 20, 2020
@mernst
Copy link
Member

mernst commented Jul 20, 2020

Thanks for this feature!

@PRITI1999 PRITI1999 deleted the fix-issue-3381 branch July 25, 2020 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mernst mernst mernst approved these changes

Assignees

@PRITI1999 PRITI1999

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

-version produces "javac (version info not available)"
2 participants
@PRITI1999 @mernst