Skip to content

Various fixes to the file related shims #983

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Oct 11, 2019
Merged

Various fixes to the file related shims #983

merged 8 commits into from
Oct 11, 2019

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Oct 11, 2019

Hi @RalfJung, I'll be working incrementally over your comments for the new fs shims module here.

@pvdrz pvdrz mentioned this pull request Oct 11, 2019
Merged
@pvdrz pvdrz changed the title [WIP] Various fixes to the fs shims [WIP] Various fixes to the file related shims Oct 11, 2019
RalfJung
RalfJung reviewed Oct 11, 2019
View reviewed changes
@RalfJung
Copy link
Member

Could you also rename the "file_manipulation.rs" test to "fs.rs"? That corresponds with the libstd module it tests.

@pvdrz
Copy link
Contributor Author

pvdrz commented Oct 11, 2019

I think we should stop this PR here and review that all the changes are adequate. This is getting too large.

@RalfJung
Copy link
Member

So, removing the WIP then?

RalfJung
RalfJung reviewed Oct 11, 2019
View reviewed changes
RalfJung
RalfJung reviewed Oct 11, 2019
View reviewed changes
RalfJung
RalfJung reviewed Oct 11, 2019
View reviewed changes
@pvdrz pvdrz changed the title [WIP] Various fixes to the file related shims Various fixes to the file related shims Oct 11, 2019
@RalfJung
Copy link
Member

Looking good aside from the nits.

@pvdrz
Copy link
Contributor Author

pvdrz commented Oct 11, 2019

Great, I'm baking another fix in the oven. Let me finish that one and I'll do this one next

@pvdrz
Copy link
Contributor Author

pvdrz commented Oct 11, 2019

I ran rustfmt by mistake, but its the last commit D: sorry

@RalfJung
Copy link
Member

I ran rustfmt by mistake, but its the last commit D: sorry

Won't this cause conflicts with all your other PRs? (And make reviewing painful.)

@RalfJung
Copy link
Member

Oh, the last commit is rustfmt plus other stuff. Please fix that.

@pvdrz
Copy link
Contributor Author

pvdrz commented Oct 11, 2019

Ok, we are good to go.

@RalfJung
Copy link
Member

Thanks! @bors r+

if you want to make PR with only rustfmt changes, I'd be up for that as well. But beware of conflicts.

@bors
Copy link
Contributor

bors commented Oct 11, 2019

📌 Commit 003b257 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Oct 11, 2019

⌛ Testing commit 003b257 with merge 6a2776e...

bors added a commit that referenced this pull request Oct 11, 2019
@bors
Auto merge of #983 - christianpoveda:fs-shims-tweaks, r=RalfJung
6a2776e 
Various fixes to the file related shims

Hi @RalfJung, I'll be working incrementally over your comments for the new `fs` shims module here.
@bors
Copy link
Contributor

bors commented Oct 11, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 6a2776e to master...

@bors bors merged commit 003b257 into rust-lang:master Oct 11, 2019
This was referenced Oct 11, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@bjorn3 bjorn3 bjorn3 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.
4 participants
@pvdrz @RalfJung @bors @bjorn3