Resolve imports - #24
Conversation
|
What are all those changes to .gitignore? Which lines of those are really needed for IntelliJ? They seem to be copy-pasted from elsewhere and I'm not sure if you are able to contribute it directly. |
|
Sorry, my mistake. It's fixed now. |
stain
left a comment
stain
left a comment
There was a problem hiding this comment.
Thank you for this effort! I've not got as far as testing the build yet, but the changes look OK, except where noted in comments. Would you be able to address those before merging?
| @@ -0,0 +1,26 @@ | |||
| package org.apache.taverna.cwl.utilities.preprocessing; | |||
There was a problem hiding this comment.
Could you add a license header here?
| @@ -0,0 +1,31 @@ | |||
| /******************************************************************************* | |||
| * Licensed to the Apache Software Foundation (ASF) under one or more | |||
There was a problem hiding this comment.
Nitpick:
indentation error in header. This seems to be true for all of your added files.
| @@ -0,0 +1,39 @@ | |||
| /******************************************************************************* | |||
| * Licensed to the Apache Software Foundation (ASF) under one or more | |||
| /** | ||
| * This class provides abstract between node importing and how it's imported | ||
| */ | ||
| public class ImportNodeImpl implements ImportNode { |
There was a problem hiding this comment.
Could you explain the reasoning for ImportNode vs ImportData? I don't think it's wrong, just a bit confused.. is it in a way so that the ImportNode is kind of the instruction to import, and ImportData the imported data?
There was a problem hiding this comment.
It's bit confusing I see it now. Let's keep ImportNode and remove ImportData
| @Override | ||
| public JsonNode importData(URI uri) { | ||
|
|
||
| try (BufferedInputStream inputStream = new BufferedInputStream(uri.toURL().openStream())) { |
There was a problem hiding this comment.
Good, I think this will support Taverna's username/password mechanism if Basic Auth is needed, or SSL certification checks if arbitrary certificates are used.
I think this would however also support climbing out and say import file:///etc/passwd
or http://intranet15:1313/ - let's see afterwards how we can be more selective about which hosts are OK to use for imports (e.g. browser-like same-origin checking and UI pop-up to confirm)
| @@ -0,0 +1,16 @@ | |||
| # Root logger option | |||
There was a problem hiding this comment.
Add ASF license header, presumably using # as comment
| ################################################################################# | ||
|
|
||
|
|
||
| cwlVersion: v1.0 |
There was a problem hiding this comment.
What's the origin of these test workflows? Did you make them, or did they come from the commonwl.org site?
There was a problem hiding this comment.
I made them for testing.
| @@ -0,0 +1,3 @@ | |||
| hello: | |||
There was a problem hiding this comment.
ASF license header missing (even if this file is really tiny..)
| *~ | ||
|
|
||
|
|
||
| # Created by https://www.gitignore.io/api/intellij,intellij+iml |
There was a problem hiding this comment.
What's the license of the output of gitignore.io? Also I don't think we really need all these ignores for intelliJ -- would not a one line ignore of .idea/ be sufficient?
| <artifactId>jackson-databind</artifactId> | ||
| <version>${jackson.version}</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
SnakeYAML is Apache License 2.0 - so OK to use. (Why not 1.18?)
There was a problem hiding this comment.
version is updated
3 participants
No description provided.