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

Implement TryFrom<std::fs::Metadata> for remotefs::fs::Metadata #11

Closed
ok-nick opened this issue Jan 2, 2022 · 3 comments
Closed

Implement TryFrom<std::fs::Metadata> for remotefs::fs::Metadata #11

ok-nick opened this issue Jan 2, 2022 · 3 comments
Assignees
Labels
enhancement New feature or request feature-request A new feature request

Comments

@ok-nick
Copy link
Contributor

ok-nick commented Jan 2, 2022

Should be an understandable change, it removes a lot of boilerplate when calling the create method of RemoteFs.

@veeso
Copy link
Member

veeso commented Jan 3, 2022

Implemented in 0.2.0 branch. Actually, I opted for From instead of TryFrom, here's the reason: it's true that when accessing to times, the getter may fail if the requested data is not supported on the current system, but it would really be a hustle to deal with it.

For example: on UNIX based systems, created() will always return an error. That's because those systems won't record the created time for files. This means that whenever I get a stdlib metadata from a remote server, the try from would fail in case the data comes from a Linux server, but wouldn't fail when received from a windows server. I can imagine that this would be kinda hard to handle for a user.

So I think the best option here, is to return ok().unwrap_or(UNIX_EPOCH).

@veeso veeso added enhancement New feature or request feature-request A new feature request labels Jan 3, 2022
@ok-nick
Copy link
Contributor Author

ok-nick commented Jan 3, 2022

I think it would be best to keep fields like created as an option. It should be up to the user to decide on how they want to handle missing timestamps. Think about a case where the user wants to display a UI representing the creation timestamp of a specified file. Well, it would say the file was created in 1970 which would be very odd.

For the TryFrom vs From case, even though the field may not be supported by the underlying os, perhaps it is supported, and the error came from some other internal issue. The user should be informed and should somehow be able to account for it. If the field is not supported then it should default to None.

@veeso
Copy link
Member

veeso commented Jan 4, 2022

Yeah, keeping times as an option is a great idea tbh

Implemented in commit a6ac32c

@veeso veeso closed this as completed Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature-request A new feature request
Projects
None yet
Development

No branches or pull requests

2 participants