Skip to content

FileLocation improvements#322

Merged
ctrueden merged 2 commits intomasterfrom
fix-filelocation-to-string
Oct 16, 2018
Merged

FileLocation improvements#322
ctrueden merged 2 commits intomasterfrom
fix-filelocation-to-string

Conversation

@gab1one
Copy link
Copy Markdown
Contributor

@gab1one gab1one commented May 6, 2018 •
edited
Loading

  • add a better toString() method
  • require non-Null arguments

@gab1one gab1one changed the title add a better toString() method to fileLocation add a better toString() method to FileLocation May 6, 2018
@gab1one gab1one changed the title add a better toString() method to FileLocation FileLocation improvements May 6, 2018
@gab1one gab1one force-pushed the fix-filelocation-to-string branch from b5ec9cf to fe64c13 Compare May 6, 2018 13:17
ctrueden
ctrueden reviewed May 14, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Let's resolve how we want toString() to work for all these location types. Then this will be easy to finalize and merge.

src/main/java/org/scijava/io/location/FileLocation.java Outdated

@Override
public String toString() {
return "FileLocation: " + file.getAbsolutePath();
Copy link
Copy Markdown
Member

@ctrueden ctrueden May 14, 2018

Choose a reason for hiding this comment

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

I'd like to be consistent about what toString() returns for the different location types, as much as we feasibly can. Would it make sense to always return a URI-style format? Or simply return file.toString() directly, perhaps? Are these strings only going to be used for debugging? Or might they appear in a UI somewhere?

Copy link
Copy Markdown
Member

@imagejan imagejan May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This might be of particular interest when calling modules/scripts headless (e.g. from the CLI) and handing over parameters as Strings, no? In that case, I'd vote for URI-style formats to be able to losslessly convert between String and Location both ways.

Copy link
Copy Markdown
Contributor Author

@gab1one gab1one May 15, 2018

Choose a reason for hiding this comment

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

We already have a mechanism to convert from String to Location via the LocationService. Also every location has a toURI method to turn it into an URI which can be turned into an Location. This toString override was meant for logging etc.

@ctrueden ctrueden added the to do label Oct 3, 2018
@gab1one gab1one force-pushed the fix-filelocation-to-string branch from fe64c13 to 1c835ab Compare October 15, 2018 13:48
ctrueden and others added 2 commits October 16, 2018 10:41
@ctrueden
AbstractLocation: implement a sensible toString()
d9cf223 
This will make it easier to understand Location objects when debugging.
@ctrueden
FileLocation: require non-Null arguments
d9eac4e
@ctrueden ctrueden force-pushed the fix-filelocation-to-string branch from 1c835ab to d9eac4e Compare October 16, 2018 15:41
@ctrueden ctrueden merged commit b31db2e into master Oct 16, 2018
@ctrueden ctrueden deleted the fix-filelocation-to-string branch October 16, 2018 15:42
@ctrueden ctrueden removed the to do label Oct 16, 2018
@gab1one
Copy link
Copy Markdown
Contributor Author

gab1one commented Oct 16, 2018

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ctrueden ctrueden ctrueden left review comments

@imagejan imagejan imagejan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gab1one @ctrueden @imagejan