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

Update FilePath constructors to allow a non-partial way to create a FilePath #3819

Merged
merged 26 commits into from
Sep 2, 2021

Conversation

rhagenson
Copy link
Member

@rhagenson rhagenson commented Aug 4, 2021

Closes #3820

As part of RFC 70, FilePath now has two constructors: create and from. Where previously create was a partial constructor and thus could fail if called with a FilePath base, now create is a complete constructor and will always succeed. Partial construction with FilePath now exists in the from constructor.

A new package-level root authority, FileAuth, has been added to allow explicit "files" authority without AmbientAuth. This new authority follows the same convention as is used in the "net" package -- note: this change is different from the the accepted RFC in order to maintain consistency within stdlib 's use of package-level root authority. Any prior instance where AmbientAuth was accepted in relation to FilePath now also accepts FileAuth with the same result.

Additional tests have been added to cover the expected error behavior of FileAuth.from(...), as well as the creation and use of FileAuth in place of AmbientAuth.

@rhagenson rhagenson added the do not merge This PR should not be merged at this time label Aug 4, 2021
@Theodus
Copy link
Contributor

Theodus commented Aug 4, 2021

Closes #3820

@SeanTAllen
Copy link
Member

I'm loiking forward to this one.

@rhagenson
Copy link
Member Author

@SeanTAllen This is (I believe) my first contribution to the stdlib so I am looking forward to it as well.

I have all the tests passing now. I still need to add the additional test(s) for the change and want to look at stdlib/tests to see if I can reduce the size of try-blocks now that create cannot error. The former is guaranteed, the latter is "with this change can we have cleaner code in stdlib" thinking.

Also kind of surprised the following in mkdtemp works:

...
    let parent: FilePath = match base
      | let base': FileAuth => FilePath(base', dir)
      | let base': FilePath => FilePath.from(base', dir)?
    end
...

I would expect parent: (FilePath | None) with FilePath.from(base', dir)? being partial. Or, alternatively FilePath.from(base', dir)? to require a try-block.

@SeanTAllen
Copy link
Member

mkdtemp is defined as partial. I assume that hasn't changed...

  new val mkdtemp(
    base: (FilePath | AmbientAuth),
    prefix: String = "",
    caps': FileCaps val = recover val FileCaps .> all() end)
    ?

and it would explain the working without a try.

@rhagenson
Copy link
Member Author

Ah, I was not thinking big enough. I had assumed the part I rewrote to now only sometimes failing would have bubbled up more, but at the function level it is already denoted as possibly failing.

@rhagenson
Copy link
Member Author

I finished the split of FilePath construction to be create with new FileAuth and from with FilePath, fixed existing code in stdlib for this change, added tests for successful and failing serial construction. Last thing I know I need to write for this is a changelog entry, but I am going to open it up for review prior to sync tomorrow.

.release-notes/split-filepath.md Outdated Show resolved Hide resolved
packages/files/auth.pony Outdated Show resolved Hide resolved
examples/mandelbrot/mandelbrot.pony Show resolved Hide resolved
examples/mandelbrot/mandelbrot.pony Outdated Show resolved Hide resolved
packages/files/_test.pony Show resolved Hide resolved
packages/files/_test.pony Outdated Show resolved Hide resolved
packages/files/_test.pony Outdated Show resolved Hide resolved
packages/files/_test.pony Outdated Show resolved Hide resolved
packages/files/_test.pony Outdated Show resolved Hide resolved
packages/files/file_path.pony Outdated Show resolved Hide resolved
@SeanTAllen SeanTAllen changed the title Split FilePath (accepted RFC) Update FilePath constructors to allow a non-erroring way to create a FilePath Aug 14, 2021
@SeanTAllen
Copy link
Member

SeanTAllen commented Aug 14, 2021

@rhagenson before this gets merged, can you update the first comment on this PR to be a good summation of the changes that this encompasses and link to the rfc? We'll use that as the squashed commit comment and if anyone follows the link from the CHANGELOG to this PR, they will find a good summary at the top.

Please include Closes #3820 as part of that reworked comment.

@SeanTAllen SeanTAllen changed the title Update FilePath constructors to allow a non-erroring way to create a FilePath Update FilePath constructors to allow a non-partial way to create a FilePath Aug 14, 2021
@SeanTAllen
Copy link
Member

@rhagenson let me know if you would like any assistance with this.

@rhagenson
Copy link
Member Author

I will make a point to get all the suggested changes in place today. I have been wrapped up in other matters so have not gotten back to this. I recognize that y'all are waiting on me so moving it up in priority. I do not think I will need assistance.

@SeanTAllen
Copy link
Member

@rhagenson no rush. was checking, nothing more.

@rhagenson
Copy link
Member Author

@SeanTAllen Top comment rewritten and all direct mentions of AmbientAuth back to using AmbientAuth (i.e., revert my FileAuth(ambient) wrappings).

Please let me know if there is anything else that is needed.

Copy link
Member

@ergl ergl left a comment

Choose a reason for hiding this comment

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

Looks good! I have left some comments below about the documentation blocks

packages/files/file_path.pony Outdated Show resolved Hide resolved
packages/files/file_path.pony Outdated Show resolved Hide resolved
.release-notes/split-filepath.md Outdated Show resolved Hide resolved
Comment on lines 8 to 10
let auth = env.root as AmbientAuth
with file = OpenFile(
FilePath(env.root as AmbientAuth, env.args(1)?, caps)?) as File
FilePath(auth, env.args(1)?, caps)) as File
Copy link
Member

Choose a reason for hiding this comment

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

I think this change with let auth temporary variable should be reverted. it hides the "actual change".

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

@SeanTAllen
Copy link
Member

This looks good to me. I added one small change request and otherwise, I do agree with @ergl's couple of notes. Looking pretty good though.

@rhagenson
Copy link
Member Author

@ergl and @SeanTAllen I took at your comments into account. While fixing the removal of let auth = env.root as AmbientAuth in Sean's comment I did another search for instances of my previously necessary FileAuth(auth) wrapping and removed more of them -- namely in process/_test.pony.

@ergl ergl self-requested a review September 2, 2021 05:20
@SeanTAllen SeanTAllen merged commit 35965c4 into main Sep 2, 2021
@SeanTAllen SeanTAllen deleted the rfc-split-filepath branch September 2, 2021 12:55
ergl added a commit that referenced this pull request Sep 2, 2021
This test was added on PR #3817, which was missed by PR #3819
since it wasn't rebased.
SeanTAllen pushed a commit that referenced this pull request Sep 2, 2021
This test was added on PR #3817, which was missed by PR #3819
since it wasn't rebased.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge This PR should not be merged at this time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Split FilePath constructor to guarantee constructor with AmbientAuth
4 participants