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

Target netstandard2.0 #391

Merged
merged 15 commits into from
Jul 17, 2018
Merged

Target netstandard2.0 #391

merged 15 commits into from
Jul 17, 2018

Conversation

zyzhu
Copy link
Contributor

@zyzhu zyzhu commented Jul 16, 2018

Various changes based on the initial efforts of @kblohm. Both Deedle and Deedle.RProvider compiles targeting dotnet standard now.

@zyzhu zyzhu changed the title Support for dotnet standard Support for netstandard2.0 Jul 16, 2018
@zyzhu
Copy link
Contributor Author

zyzhu commented Jul 16, 2018

Trigger travis build.

@zyzhu zyzhu closed this Jul 16, 2018
@zyzhu zyzhu reopened this Jul 16, 2018
@zyzhu zyzhu changed the title Support for netstandard2.0 Target netstandard2.0 Jul 16, 2018
@zyzhu zyzhu merged commit 259d933 into fslaborg:master Jul 17, 2018
@dsyme
Copy link
Member

dsyme commented Jul 18, 2018

THis is great work. @kblohm Could you do a post-hoc review? thanks

@kblohm
Copy link
Contributor

kblohm commented Jul 22, 2018

Sure i can.
Just a few minor notes:

  • all the Tests should probably target netcoreapp2.1 instead of 2.0.
  • the fix for the documentation-test does not work for non-english languages because the error messages get translated. I, for example, can not run them.
  • i can not run the RPlugin-Tests or even compile them (not even on net45). Not sure if i am missing something there or if you have the same problem.
  • in my opinion, the the RPlugin should not target netstandard until the RProvider actually supports it. At least if you plan on releasing a version 2.0.0-alpha soon, i would just release Deedle without it and provide the RPlugin later.

@zyzhu
Copy link
Contributor Author

zyzhu commented Jul 25, 2018

@kblohm Thanks for your review. I've done a pull request to address some problems. Please take a look and let me know what you think.
#393

    • All Deedle tests target netcoreapp2.1 and net45
    • RPlugin.tests target net45
    • Documentation.tests target net461 because of fsharp.formatting
  1. I don't know how to force locale of error messages to en-us. I've tried some tricks but didn't succeed. Maybe a short-term solution is to turn off documentation test on your local machine so that build can pass.
    • RPlugin.Tests is really a pain. My environment has R-3.4.3 (with zoo package installed) installed at C:\R\R-3.4.3\bin\x64. I add this to path environment variable. I also set R_HOME to be C:\R\R-3.4.3. With those environment settings, RPlugin.Tests shall be able to build.
    • But in order to run RPlugin.Tests, I couldn't get it working via dotnet test as it always look for x86 version of R by default. For now, I enforced RPlugin.Tests to be run via nunit.consolerunner.
    • I also changed the default setting in build.fsx. By default it will only build allcore which exclude rplugin and rplugin tests so that we can focus on Deedle itself until the picture of RProvider is clearer.
  2. I did remove netstandard from Deedle.RPlugin. We shall keep it at net45 until RProvider is on netstandard.

@zyzhu zyzhu deleted the netstandard branch July 27, 2018 19:31
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.

3 participants