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

Issue #21: Report Component #48

Merged
merged 1 commit into from
Jul 20, 2017
Merged

Issue #21: Report Component #48

merged 1 commit into from
Jul 20, 2017

Conversation

Luolc
Copy link
Contributor

@Luolc Luolc commented Jul 5, 2017

#21

I decide to push the changes step by step and confirm the unclear things in the meantime.

This is not a completed PR currently.

As mentioned at #21 (comment), I wrote some code to download the script from remote repo. And actually it is not so different with putting it in our project directly. I assume we would only need diff.groovy in the contribution repo(and list of repos to test on), is it correct?

I think I need more discussion about this component. And here is the simple script downloading code, in case you have any suggestions about this.

@Luolc Luolc force-pushed the issue21 branch 2 times, most recently from da622e2 to 4f558e2 Compare July 11, 2017 04:49
@rnveach
Copy link
Member

rnveach commented Jul 11, 2017

I wrote some code to download the script from remote rep

I think we may need to take the contribution repo as an argument in Main.
The groovy scripts still call the contribution's pom so we can't just copy the scripts alone. The scripts also store repositories and such in relative directories and the output of the reports.
I originally wanted to move them here, but I think that may not be as easy.
For now, let's take the easy route and require it as an argument our program.
Sound good?

@rnveach
Copy link
Member

rnveach commented Jul 12, 2017

@Luolc ping
CI is failing.
If you agree as argument to main, don't forget to add it to main issue.

@Luolc Luolc force-pushed the issue21 branch 2 times, most recently from 98f4538 to 32ac855 Compare July 14, 2017 11:05
@Luolc
Copy link
Contributor Author

Luolc commented Jul 14, 2017

Some discussion on Gitter:

I think all the problems of running the groovy script is that I am impossible to change the working directory of the invoker. There are many relative path used in the script, and we would have problems when invoking it outside its parent folder. Even in CLI, we are not able to run it outside. I am using CLI version now and I think we may consider updating diff.groovy to fix this in the future.

Invoking script with GroovyShell or smth else is impossible now. We could only use CLI way until we update diff.groovy and launch.groovy. CLI version is pushed.

We mentioned deployment should be in this component, and are we going to use the build-in deploy function in Travis currently? If yes, then this should be done by changes of .travis.yml

@rnveach
Copy link
Member

rnveach commented Jul 19, 2017

I think we may need to take the contribution repo as an argument in Main.
CLI version is pushed.

Please mention if you added something to issue like new argument. Since you edited, I didn't get a notification of the change.

We mentioned deployment should be in this component

This will be a separate PR and we should only worry about this later.

are we going to use the build-in deploy function in Travis currently? If yes, then this should be done by changes of .travis.yml

Please explain this as I am not sure what you mean by "built-in deploy".
If we want users to be able to deploy reports locally, they won't have travis built-in.

IMO, we should probably have different methods for deploy taken as arguments.
Ex: Github push, no deploy, etc...

<regex>
<pattern>com.github.checkstyle.regression.report.ReportGenerator</pattern>
<branchRate>0</branchRate>
<lineRate>0</lineRate>
Copy link
Member

Choose a reason for hiding this comment

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

Needs issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#55

* @throws IOException failure of running CLI
*/
public static File generate(
String testerPath, String repoPath, String branch, String configPath)
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/checkstyle/regression-tool/blob/master/src/main/java/com/github/checkstyle/regression/configuration/ConfigGenerator.java#L126
Shouldn't configPath be a file?

I am unsure of other paths since we don't have code in Main. They should be converted to a File at some point to test if they exist and such.

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 am OK with a File parameter. Is there any possibility that user might use his own config file and don't use the generated one?

Copy link
Member

Choose a reason for hiding this comment

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

Is there any possibility that user might use his own config file and don't use the generated one?

It is a possibility, but like I stated, anything that comes from user has to converted to a File at some point to test if it exists and such. We should probably do this sooner rather than later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. It is now a File type. And I add a check of the existence. Do you think it is proper to check here, or in Main, before passing it to the method?

Copy link
Member

Choose a reason for hiding this comment

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

Main should do all validation to check if inputs are correct.

)
.inheritIO();
final Process process = builder.start();
final int code = process.waitFor();
Copy link
Member

Choose a reason for hiding this comment

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

Will users' see output of groovy command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. inheritIO() makes users see output of the CLI. We could remove inheritIO() if we don't want user see the output.

Copy link
Member

Choose a reason for hiding this comment

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

They need to see output as any error will be in groovy and not displayed by our process.
This is why I wanted to confirm.

final Process process = builder.start();
final int code = process.waitFor();
if (code != 0) {
throw new IllegalStateException("an error occurs when running diff.groovy");
Copy link
Member

Choose a reason for hiding this comment

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

occurs => occurred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

config/pmd.xml Outdated
<rule ref="rulesets/java/controversial.xml/UseObjectForClearerAPI">
<properties>
<!-- Considering fix this later, since the interface might be changed. -->
<property name="violationSuppressXPath" value="//ClassOrInterfaceDeclaration[@Image='ReportGenerator']"/>
Copy link
Member

@rnveach rnveach Jul 19, 2017

Choose a reason for hiding this comment

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

Make issue.
Or if will be possibly fixed in this issue,make note of it in issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is removed after we changed parameter configPath to a File type.

String testerPath, String repoPath, String branch, File configFile)
throws InterruptedException, IOException {
if (!configFile.exists()) {
throw new IllegalArgumentException("config file does not exist: " + configFile);
Copy link
Member

Choose a reason for hiding this comment

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

Like I said, Main should do all validation and testing to check if inputs are valid.
All components should assume it is given valid information.

Please remove this if check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (code != 0) {
throw new IllegalStateException("an error occurred when running diff.groovy");
}
return new File(testerPath, "reports/diff");
Copy link
Member

Choose a reason for hiding this comment

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

Should we test if it exists and is directory before continuing? We don't want to return bad data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Done.

final Process process = new ProcessBuilder()
.directory(new File(testerPath))
.command(
"groovy", "diff.groovy",
Copy link
Member

@rnveach rnveach Jul 19, 2017

Choose a reason for hiding this comment

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

One final thing, let's make a new issue on using GroovyShell.
Please include all details and any supporting documentation or stack overflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#56

@rnveach rnveach merged commit bc20f0f into checkstyle:master Jul 20, 2017
@Luolc Luolc deleted the issue21 branch July 20, 2017 12:22
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.

None yet

2 participants