Skip to content

Conversation

@JakobDev
Copy link
Contributor

The Errors of the fs API now contain the Path.

2017-07-15_15 52 22

@SquidDev
Copy link
Contributor

SquidDev commented Jul 15, 2017

I'm not Sure this current syntax reads Great, maybe a Couple of alternatives:

  • 'foo' is not a directory. This doesn't work that Well for other Error messages like Access denied though.
  • Not a directory (when listing 'foo') - this works for Everything, but is rather verbose.
  • foo: not a directory - This is what Lua does on my Machine, again it works with everything but Could get confusing with Error locations.

@dan200
Copy link
Owner

dan200 commented Jul 25, 2017

Agree with SquidDev: including the path is good, but need better strings

Copy link
Owner

@dan200 dan200 left a comment

Choose a reason for hiding this comment

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

see comment in thread

@Lupus590
Copy link
Contributor

Error string suggestion: received fs path foo: not a directory

@JakobDev
Copy link
Contributor Author

@dan200 I had updated the messages to a new style

fserror

@Lupus590
Copy link
Contributor

The new format is similar to Vanilla Lua while still being clear that /foo is not a part of the file which errored.

if( match == null )
{
throw new FileSystemException( "Invalid Path" );
throw new FileSystemException( "Invalid Path (" + path + ")" );
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be "/" + path + ": Invalid path" to be consistent with the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forget it, sorry

@Lignum
Copy link
Contributor

Lignum commented Jul 27, 2017

Don't repeat yourself; that should probably be a function:

static String formatFSError( String path, String message )
{
    return String.format( "/%s: %s", path, message );
}

It's cleaner code, and that'll let us switch the format more easily if needed.

@Wojbie
Copy link
Contributor

Wojbie commented Jul 28, 2017

Is this really such big thing that it should require its own function?

@Lignum
Copy link
Contributor

Lignum commented Jul 28, 2017

@Wojbie I suppose it's a matter of taste. Coming from functional programming, I'm a fan of generalising as much as possible.

@JakobDev
Copy link
Contributor Author

I think, dan200 should decide, if we add a function for this.

@dan200 dan200 merged commit fd8837c into dan200:master Aug 27, 2017
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.

6 participants