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

CLI and TaskExecutor #5

Closed
wants to merge 1 commit into from
Closed

CLI and TaskExecutor #5

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 30, 2015

In future could improve CLI to allow multiple projects/tasks in one command.

jamo
jamo reviewed Mar 30, 2015
View reviewed changes
tmc-langs-util/src/main/java/fi/helsinki/cs/tmc/langs/util/JsonWriter.java

private static String buildJson(Object obj) {
Gson gson = new Gson();
return gson.toJson(obj);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just: return new Gson().toJson(obj);

Copy link
Author

Choose a reason for hiding this comment

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

Removed method and instead use String objectInJson = new Gson().toJson(obj);

@jamo
Copy link
Member

jamo commented Mar 30, 2015

Regarding file: tmc-langs-util/src/test/resources/jsonwriter_files/test.json; maybe we can gitignore it.

mpartel
mpartel reviewed Apr 3, 2015
View reviewed changes
tmc-langs-util/src/main/java/fi/helsinki/cs/tmc/langs/util/JsonWriter.java
try {
writeOutputFile(objectInJson, outputFile);
} catch (IOException ex) {
Logger.getLogger(JsonWriter.class.getName()).log(Level.SEVERE, null, ex);
Copy link
Member

Choose a reason for hiding this comment

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

Bad idea to not rethrow exceptions in a generic helper like this. I also don't think it's necessary to log the exception here - let whatever handles the final exception do logging.

Copy link
Author

Choose a reason for hiding this comment

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

Factored the two methods into one and the method now throws the exception to be handled later.

@mpartel
Copy link
Member

mpartel commented Apr 3, 2015

When I run mvn verify in the top level tmc-langs directory, I get:

org.apache.maven.surefire.util.SurefireReflectionException: java.lang.reflect.InvocationTargetException; nested exception is java.lang.reflect.InvocationTargetException: null
java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:164)
        at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:110)
        at org.apache.maven.surefire.booter.SurefireStarter.invokeProvider(SurefireStarter.java:175)
        at org.apache.maven.surefire.booter.SurefireStarter.runSuitesInProcessWhenForked(SurefireStarter.java:107)
        at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:68)
Caused by: java.lang.NoClassDefFoundError: trivial/build/test/classes/TrivialTest (wrong name: TrivialTest)
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(ClassLoader.java:760)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:467)
        at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        at org.apache.maven.surefire.util.DefaultDirectoryScanner.loadClass(DefaultDirectoryScanner.java:98)
        at org.apache.maven.surefire.util.DefaultDirectoryScanner.locateTestClasses(DefaultDirectoryScanner.java:78)
        at org.apache.maven.surefire.junit4.JUnit4Provider.scanClassPath(JUnit4Provider.java:174)
        at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:83)
        ... 9 more

@mpartel
Copy link
Member

mpartel commented Apr 3, 2015

Review done. Sorry, it took me way too long to get to this.

@ghost
Copy link
Author

ghost commented Apr 11, 2015

The errors with mvn verify should be fixed now, Travis us running mvn verify now as well.

jamo added a commit that referenced this pull request Aug 31, 2015
@jamo
Merge pull request #5 from rage/more-travis
36ec172 
Start using the new container based travis for faster build results
@ljleppan
Copy link
Member

This branch seems stale.

While we do need a better README.MD than what we have now, I'd suggest we remove this branch and redo the work so that it's assured to be up to date.

@jamo
Copy link
Member

jamo commented Sep 30, 2015

I'll cherrypick some of the changes and then close the pr

@jamo jamo self-assigned this Sep 30, 2015
@jamo
Copy link
Member

jamo commented Nov 6, 2015

Some of the commits manually added to master.

@jamo jamo closed this Nov 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jamo jamo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jamo @mpartel @ljleppan @PunyW