Skip to content

Various fixes to the file related shims#983

Merged
bors merged 8 commits into
rust-lang:masterfrom
pvdrz:fs-shims-tweaks
Oct 11, 2019
Merged

Various fixes to the file related shims#983
bors merged 8 commits into
rust-lang:masterfrom
pvdrz:fs-shims-tweaks

Conversation

@pvdrz

@pvdrz pvdrz commented Oct 11, 2019

Copy link
Copy Markdown
Contributor

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

@pvdrz pvdrz changed the title [WIP] Various fixes to the fs shims [WIP] Various fixes to the file related shims Oct 11, 2019
Comment thread src/helpers.rs
Comment thread src/shims/env.rs Outdated
Comment thread tests/run-pass/current_dir.rs Outdated
Comment thread tests/run-pass/current_dir.rs Outdated
@RalfJung

Copy link
Copy Markdown
Member

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

@pvdrz

pvdrz commented Oct 11, 2019

Copy link
Copy Markdown
Contributor Author

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

@RalfJung

Copy link
Copy Markdown
Member

So, removing the WIP then?

Comment thread src/helpers.rs Outdated
Comment thread src/helpers.rs Outdated
Comment thread src/shims/env.rs Outdated
@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
Copy Markdown
Member

Looking good aside from the nits.

@pvdrz

pvdrz commented Oct 11, 2019

Copy link
Copy Markdown
Contributor Author

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

@pvdrz

pvdrz commented Oct 11, 2019

Copy link
Copy Markdown
Contributor Author

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

@RalfJung

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@pvdrz

pvdrz commented Oct 11, 2019

Copy link
Copy Markdown
Contributor Author

Ok, we are good to go.

@RalfJung

Copy link
Copy Markdown
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

bors commented Oct 11, 2019

Copy link
Copy Markdown
Contributor

📌 Commit 003b257 has been approved by RalfJung

@bors

bors commented Oct 11, 2019

Copy link
Copy Markdown
Contributor

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

bors added a commit that referenced this pull request Oct 11, 2019
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

bors commented Oct 11, 2019

Copy link
Copy Markdown
Contributor

☀️ 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
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.

4 participants