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

File IO #32668

Merged
merged 121 commits into from Dec 18, 2018

Conversation

@paul1956
Copy link
Contributor

paul1956 commented Oct 7, 2018

This added VB FileIO and MsbBox and support, on Windows it includes UI support on other OS's it throws exception on attempt to use UI.

@paul1956 paul1956 changed the title File io update1 File IO and MsgBox Oct 8, 2018

@paul1956

This comment has been minimized.

Copy link
Contributor Author

paul1956 commented Oct 8, 2018

Ping @333fred this is stable enough to review, there is also a Merge Conflict I am not sure what to do with. I just added lines to a file, I didn't delete or move any lines.

@333fred

This comment has been minimized.

Copy link
Member

333fred commented Oct 8, 2018

@paul1956 is there a bug associated with this change?

@paul1956

This comment has been minimized.

Copy link
Contributor Author

paul1956 commented Oct 8, 2018

@333fred #31181 (comment)
Port remainder of Microsoft.VisualBasic

paul1956 added some commits Oct 9, 2018

Move Constants out of Globals where they don't belong. Restore 3 Reso…
…lution functions to unmodified state. Fix issues in Interaction Module.
@333fred

This comment has been minimized.

Copy link
Member

333fred commented Oct 10, 2018

@paul1956 is this ready for review? If you're still working on it I don't want to go through it. As a general comment though, you need to resolve the conflicts in Microsoft.VisualBasic.cs. Additionally, I started looking an immediately noticed some changes that are purely reformatting things, such as moving Microsoft.VisualBasic.CompareMethod around and making no other changes. Please make sure that you're not moving things around like that, as it will make it harder to track who actually wrote/changed a line.

@paul1956

This comment has been minimized.

Copy link
Contributor Author

paul1956 commented Oct 10, 2018

@333fred I have no idea how to resolve conflicts in that file without some help, VS shows no conflicts, and all I did was add some lines which should not have caused any conflicts (which is what GitHub shows). I originally moved CompareMethod to try and resolve conflict with change that added ComClassAttribute. FileIO and MsgBox could be reviewed I don't have any tests for TextFieldParse and was hoping someone could point me at the original tests for that Module. Also some functions are just straight passthroughs to System.IO, how to I signify that? Do I include a empty test with a comment, leave it out or something else?

paul1956 added some commits Oct 11, 2018

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Oct 11, 2018

Also some functions are just straight passthroughs to System.IO, how to I signify that? Do I include a empty test with a comment, leave it out or something else?

For each such APi I would add one trivial test just sufficient that if someone edited the VB implementation to remove that pass through to System.IO, the test would break. For example, if the VB function is calling System.IO.File to open a file, I would have a test that called the VB function with an invalid path, and check you get the appropriate exception.

@333fred

This comment has been minimized.

Copy link
Member

333fred commented Oct 11, 2018

I don't have any tests for TextFieldParse and was hoping someone could point me at the original tests for that Module

These are the old tests we know about: https://github.com/danmosemsft/corefx/tree/old.vb.tests. If there are others that are open source, we're not aware of them.

Also @cston.

Enum FormatType
ErrorFormat = 0
StdFormat = 1
End Enum

This comment has been minimized.

@cston

cston Nov 26, 2018

Member

Consider replacing with simple useErrorFormat As Boolean in MultiFormatTest.

This comment has been minimized.

@paul1956

paul1956 Dec 5, 2018

Author Contributor

Again this was an original Framework test from MSDN but I will change.

@paul1956 paul1956 changed the title File IO and MsgBox File IO Dec 5, 2018

@paul1956

This comment has been minimized.

Copy link
Contributor Author

paul1956 commented Dec 5, 2018

I have removed MsgBox and it can be in a separate PR

@cston

cston approved these changes Dec 14, 2018

@paul1956

This comment has been minimized.

Copy link
Contributor Author

paul1956 commented Dec 14, 2018

@cston Thanks, I would like to start another PR to update this to use Windows.Forms so that the functions I disabled would will work but not sure how to include Windows Forms since it is a separate solution. Any guidance would be appreciated.

@cston

This comment has been minimized.

Copy link
Member

cston commented Dec 14, 2018

@danmosemsft is the failure of the packaging build expected? Thanks.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Dec 14, 2018

D:\j\workspace\windows-TGrou---0d2c9ac4\.packages\microsoft.dotnet.build.tasks.packaging\1.0.0-beta.18613.4\build\Packaging.targets(420,5): error : Package contains two files with same targetpath: ref/netstandard2.0/Microsoft.VisualBasic.dll, items:D:\j\workspace\windows-TGrou---0d2c9ac4\artifacts\bin\ref\Microsoft.VisualBasic\netstandard-Windows_NT\Microsoft.VisualBasic.dll, D:\j\workspace\windows-TGrou---0d2c9ac4\artifacts\bin\ref\Microsoft.VisualBasic\netstandard-Unix\Microsoft.VisualBasic.dll. [D:\j\workspace\windows-TGrou---0d2c9ac4\src\Microsoft.VisualBasic\pkg\Microsoft.VisualBasic.pkgproj]
D:\j\workspace\windows-TGrou---0d2c9ac4\.packages\microsoft.dotnet.build.tasks.packaging\1.0.0-beta.18613.4\build\Packaging.targets(420,5): error : Package contains two files with same targetpath: ref/netstandard2.0/Microsoft.VisualBasic.pdb, items:D:\j\workspace\windows-TGrou---0d2c9ac4\artifacts\bin\ref\Microsoft.VisualBasic\netstandard-Windows_NT\Microsoft.VisualBasic.pdb, D:\j\workspace\windows-TGrou---0d2c9ac4\artifacts\bin\ref\Microsoft.VisualBasic\netstandard-Unix\Microsoft.VisualBasic.pdb. [D:\j\workspace\windows-TGrou---0d2c9ac4\src\Microsoft.VisualBasic\pkg\Microsoft.VisualBasic.pkgproj]
D:\j\workspace\windows-TGrou---0d2c9ac4\.packages\microsoft.dotnet.build.tasks.packaging\1.0.0-beta.18613.4\build\Packaging.targets(420,5): error : Package contains two files with same targetpath: ref/netstandard2.0/Microsoft.VisualBasic.xml, items:D:\j\workspace\windows-TGrou---0d2c9ac4\.packages\microsoft.private.intellisense\3.0.0-preview1-27128-0\xmldocs\netcoreapp\1033\Microsoft.VisualBasic.xml, D:\j\workspace\windows-TGrou---0d2c9ac4\.packages\microsoft.private.intellisense\3.0.0-preview1-27128-0\xmldocs\netcoreapp\1033\Microsoft.VisualBasic.xml. [D:\j\workspace\windows-TGrou---0d2c9ac4\src\Microsoft.VisualBasic\pkg\Microsoft.VisualBasic.pkgproj]
<ItemGroup>
<ProjectReference Include="..\..\src\Microsoft.VisualBasic.vbproj" />

This comment has been minimized.

@danmosemsft

danmosemsft Dec 14, 2018

Member

I don't believe there should be an explicit project reference, it's implicitly referencing the implementation. Having said that though this project is in a non standard location.

This comment has been minimized.

@paul1956

paul1956 Dec 15, 2018

Author Contributor

It doesn't build if you can't include the explicit reference, I reported this when I started also the non-standard location causes some issues also reported. I was unable to move it without causing new problems that I filed. The C# and test VB files should probably be in the same directory with 2 project files but this goes against being able to build based on the directory structure without listing the files.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Dec 14, 2018

@cston I'm not sure but my guess is that it's because of the project references in the test projects. Our test projects should have implicit references to the implementation.

Note that you can test this leg locally with build -BuildAllConfigurations (of course it takes a long time)

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Dec 14, 2018

@ericstj does that sound right? Note it is a bit fiddly because the test .vbproj is not in the "standard" location for a test project. However @cston it should be easy to see whether it builds OK without that project reference.

@@ -2,7 +2,8 @@
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
<BuildConfigurations>
netstandard;
netstandard-Unix;

This comment has been minimized.

@ericstj

ericstj Dec 14, 2018

Member

Reference assemblies cannot differ by runtime. Runtime is never considered when resolving files that get passed to the compiler.

@ericstj

This comment has been minimized.

Copy link
Member

ericstj commented Dec 14, 2018

The packaging build is broken by the configurations for the ref project: #32668 (comment). References cannot have a RID so the two configurations get packed at the same path.

@@ -2,7 +2,8 @@
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
<BuildConfigurations>
netstandard;
netstandard-Unix;

This comment has been minimized.

@ericstj

ericstj Dec 14, 2018

Member

We want to keep a RID-less implementation since this goes into a package. Can we rename this configuration from netstandard-Unix to just netstandard? Then below give the FileSystem.Unix.vb file a different name.

@cston

This comment has been minimized.

Copy link
Member

cston commented Dec 14, 2018

I would like to start another PR to update this to use Windows.Forms

@paul1956 please hold off on additional PRs. We expect to rename the Microsoft.VisualBasic package and that may affect the directory structure. And we may move the tests written in VB to C# to simplify the structure.

@paul1956

This comment has been minimized.

Copy link
Contributor Author

paul1956 commented Dec 15, 2018

If you want/expect VB developers to contribute the tests should be in VB, this was also discussed below #31181 (comment)

cston added some commits Dec 17, 2018

@cston cston merged commit 87089ef into dotnet:master Dec 18, 2018

13 checks passed

Linux arm Release Build Build finished.
Details
Linux arm64 Release Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
NETFX x86 Release Build Build finished.
Details
OSX x64 Debug Build Build finished.
Details
Packaging All Configurations x64 Debug Build Build finished.
Details
Tizen armel Debug Build Build finished.
Details
UWP CoreCLR x64 Debug Build Build finished.
Details
UWP NETNative x86 Release Build Build finished.
Details
Windows x64 Debug Build Build finished.
Details
Windows x86 Release Build Build finished.
Details
license/cla All CLA requirements met.
Details
@cston

This comment has been minimized.

Copy link
Member

cston commented Dec 18, 2018

Thanks @paul1956. The PR has been merged.

@paul1956 paul1956 deleted the paul1956:FileIO1 branch Dec 20, 2018

@karelz karelz added this to the 3.0 milestone Dec 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.