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

Fork for issue #63 #72

Closed
wants to merge 8 commits into from
Closed

Fork for issue #63 #72

wants to merge 8 commits into from

Conversation

LasAtt
Copy link

@LasAtt LasAtt commented Sep 2, 2015

Fixes issue #63

@@ -331,7 +331,7 @@ public Path getExerciseChecksumCacheLocation() {
public ListenableFuture<URI> pasteWithComment(Path path, String comment)
throws TmcCoreException {
//checkParameters(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have been removed earlier (hups :D ), get rid of it. No commented code.

@LasAtt
Copy link
Author

LasAtt commented Sep 4, 2015

I removed all throws-clauses from ExerciseSubmitter. One thing that strikes me as odd in the current implementation though is that while URIs are now used inside ExerciseSubmitter, when communicating with UrlHelper and UrlCommunicator, URIs have to be converted to Strings or vice versa. Should the rest of the classes in core.communication-package also be refactored to use URIs in place of Strings?

@jamo
Copy link
Member

jamo commented Sep 4, 2015

Yes.
We should always prefer URI and Path over String.

@ljleppan
Copy link
Member

ljleppan commented Sep 4, 2015

Yes, we'd like to refactor all the Stringly typed code to use proper classes instead (see f.ex. testmycode/tmc-netbeans#111 and #70)

It seems we forgot to actually create issues for this prior to the course's start. I've now created issues #77 and testmycode/tmc-netbeans#115.

import fi.helsinki.cs.tmc.langs.util.TaskExecutor;
import fi.helsinki.cs.tmc.langs.util.TaskExecutorImpl;

import com.google.common.base.Optional;
import edu.emory.mathcs.backport.java.util.Collections;
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 the wrong Collections library to use.

@ljleppan
Copy link
Member

What's the status of this?

Based on previous comments and a quick skim this looks like ready to merge, as soon as it is rebased on top of the current master to remove any conflicts.

@LasAtt
Copy link
Author

LasAtt commented Sep 30, 2015

Other than the rebasing, should be ready. I'll rebase later today and post an another comment to notify when it's done.

@LasAtt
Copy link
Author

LasAtt commented Sep 30, 2015

Alright, rebasing done. Let me know if there's more errors and I'll make sure to fix them. Sorry for holding this issue so long.

@jamo jamo closed this May 18, 2016
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.

4 participants