Skip to content

Conversation

@smoothdeveloper
Copy link
Contributor

by generating the actual file next to the expected one, which can then be diffed.

I just moved the contents, there is no change in the surface area. Hopefully green.

…to the expected one, which can then be diffed.

I just moved the contents, there is no change in the surface area. Hopefully green.
Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

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

We should not be extending IFileSystem with actual file read/write functionality.
There is StreamExtensions type in the FileSystem.FS which will allow you to do the same (FileSystem.OpenFileForWrite(path).Write(contents)).

@smoothdeveloper
Copy link
Contributor Author

@vzarytovskii makes sense and done.

There is only one downside I see with raw Stream extensions versus having an abstraction that would remove the concern of 'use/IDisposable' being handled properly.

This may be improved by having extensions to IFileSystem that would deal with it, taking a Stream -> 't function instead of returning the raw Stream.

Since this is going to be used in all tooling, turning the API this way (for some use cases at least) would reduce concern for tooling on top of FCS to misuse the resources the concrete implementation returns.

@smoothdeveloper
Copy link
Contributor Author

Green.

@Happypig375
Copy link
Member

Bump

@smoothdeveloper
Copy link
Contributor Author

Hey @Happypig375, not sure this is a priority for being merged.

I like the change because it skips recompiles but it is not a big deal and probably not urgent for the maintainers to adjust it (also, conficting files now, so can't be merged as is).

I had the pending PR with init properties which is more important that I intended to update based off this, but I can also unblock it by manually adjusting the surface area .fs file there.

@KevinRansom KevinRansom changed the title Simplify FCS SurfaceAreaTest test [WIP] -Simplify FCS SurfaceAreaTest test Jul 12, 2021
@vzarytovskii
Copy link
Member

@smoothdeveloper Sorry it took so long. I've fixed the build + merged the main. If it looks ok to you, going to merge once green.

@vzarytovskii vzarytovskii changed the title [WIP] -Simplify FCS SurfaceAreaTest test Simplify FCS SurfaceAreaTest test Jul 13, 2021
@smoothdeveloper
Copy link
Contributor Author

@vzarytovskii thanks! it looks green & good to me.

@vzarytovskii vzarytovskii merged commit e5920c1 into dotnet:main Jul 13, 2021
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