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.

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

Fix Shell.mv #2309

Merged
merged 8 commits into from May 1, 2019
Merged

Fix Shell.mv #2309

merged 8 commits into from May 1, 2019

Conversation

fangyi-zhou
Copy link
Contributor

Description

Fixes #2293

TODO

  • New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in help/markdown)
  • unit or integration test exists (or short reasoning why it doesn't make sense)
  • boy scout rule: "leave the code behind in a better state than you found it" (fix warnings, obsolete members or code-style in the places you worked in)
  • (if new module) the module has been linked from the "Modules" menu, edit help/templates/template.cshtml, linking to the API-reference is fine.
  • (if new module) the module is in the correct namespace
  • (if new module) the module is added to Fake.sln (dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj)
  • Fake 5 API guideline is honored

matthid and others added 3 commits April 28, 2019 17:23
@matthid
Release 5.13.3 (#2306)
30b0949 
Release 5.13.3
@fangyi-zhou
Fix Shell.mv #2293
3d211f2
@fangyi-zhou @matthid
Add integration test for Shell.mv
05dcbb5 
Co-authored-by: Matthias Dittrich <matthi.d@gmail.com>
@fangyi-zhou
Copy link
Contributor Author

Moving for directories isn't working...

@matthid matthid mentioned this pull request Apr 30, 2019
Closed
@matthid
Copy link
Member

matthid commented Apr 30, 2019

a good start, thanks for taking a look at this.

Just FYI some tests are still failing, what I use is something like dotnet run --project src/test/Fake.Core.IntegrationTests/ -- --run "Fake.IO.FileIntegraionTests/Shell.mv works as expected when src is a dir dst is a dir - #2293" to run them locally one-by one and have the dotnet tooling rebuild everything if needed.

@fangyi-zhou fangyi-zhou marked this pull request as ready for review May 1, 2019 11:07
@fangyi-zhou fangyi-zhou changed the title Fix mv Fix Shell.mv May 1, 2019
matthid
matthid previously approved these changes May 1, 2019
View reviewed changes
Copy link
Member

@matthid matthid 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, thanks a lot for taking care of this. I can take care of the remaining things, just let me know if you want to do it or if you disagree with anything.

src/app/Fake.IO.FileSystem/FileSystemInfo.fs Outdated
@@ -25,3 +25,9 @@ module FileSystemInfo =
| :? FileInfo as file -> File(file)
| :? DirectoryInfo as dir -> Directory(dir, dir.EnumerateFileSystemInfos())
| _ -> failwith "No file or directory given."

let moveTo (fileSysInfo: FileSystemInfo) dest =
Copy link
Member

Choose a reason for hiding this comment

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

We should either have docs or mark this internal (which is what I probably would go with for now).
If we want to add this public API we should briefly discuss the order of the arguments.

What I mean: We should decide if we expect this API to be used like FileSystemInfo.moveTo src dst or like

src
|> FileSystemInfo.moveTo dst

in which case we have to re-order the arguments.

src/app/Fake.IO.FileSystem/Shell.fs
match fi with
| FileSystemInfo.Directory (d, _) ->
let targetName = target @@ fi.Name
d.MoveTo(targetName) |> ignore
Copy link
Member

Choose a reason for hiding this comment

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

can we write ignore<DirectoryInfo> here (or whatever MoveTo returns here)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method returns void, so I guess we don't need to ignore?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed

src/app/Fake.IO.FileSystem/Shell.fs Outdated
let fi_src = FileSystemInfo.ofPath src
let fi_dest = FileSystemInfo.ofPath dest
if not fi_dest.Exists then rename dest src
else begin
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 begin and end can be removed

@matthid matthid merged commit 74f4496 into fsprojects:release/next May 1, 2019
@matthid
Copy link
Member

matthid commented May 1, 2019

Thanks!

@fangyi-zhou
Copy link
Contributor Author

Please revert... that code was not compiling... Sorry

@matthid
Copy link
Member

matthid commented May 1, 2019

It's ok, will fix and start the release process :)
I have to edit the release notes anyway and wanted to try some refactoring I wasn't sure if it would work in the review ;)

@matthid
Copy link
Member

matthid commented May 1, 2019

Honestly, I didn't expect mv to be such a nasty little function :D

@matthid matthid mentioned this pull request May 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid 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.

Shell.mv doesn't move correctly
2 participants
@fangyi-zhou @matthid