Refactor Entire Project into .NET Standard #284
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d 2 project with a unit test.
|
Awsome work! Tried referencing the project form a Xamarin Forms (.Net Standards) project, Clean Master detail project, with added parse init - gives one error on Android and one on iOS. VS.NET 2017 Pro all updates on Win10 fall creators update. Android iOS Tried building earlier today on VS.NET for Mac but got some Linker errors there. Not the same as on Windows. EDIT: Parse-project alone compiles fine on VS2017 on both Mac and Windows |
|
@danibjor That's really weird; the whole reason I rewrote PCLStorage into StandardStorage is to get rid of all references to |
|
@danibjor Also, what version of the iOS and Android SDKs are you running? Look at the following document to see what the minimum versions are for each SDK when referencing .NET Standard. https://github.com/dotnet/standard/blob/master/docs/versions.md |
|
If I remember correct (vs on win) It did not crash when I just referenced the parse library. The parse solution alone builds just fine on both Mac and Windows. Same when I include the parse project in my solution. Calling parse init makes the build/deploy fail (using iOS device, vs.net on win (parallels desktop), Iphone connected to paired Mac/build host). I’ll do some more research/poke around tomorrow - and post my findings. |
|
@danibjor Thanks for offering your help. From what I can tell, this issue may be due to my reference of |
TheFanatr
changed the title
|
Edit: Also fetched the latest bits from your GitHub page before testing. Tried again today. Here is what I did so far to reproduce the errors. Setup: File, new project, C#, Cross Platform, Cross plarform app (Xamarin Forms), Master/Detail app, Strategy: .NET Core So far so good. Now, I tried two methods to get this running. Adding the dll as reference, and adding the project to my solution. Both error out. Only thing added to the default Xamarin solution is in App Init where I did a use a reference to Parse. I get two errors. On on the Android project and one on the iOS project. Android: iOS: System Info: |
|
@danibjor Very strange. My theory is that the external assembly that I packaged with StandardStorage, which is referenced by Parse, tries to use |
|
Peeking into the libraries referenced - and there sure is a reference to System.Windows.Forms
|
|
I noticed as well, and I actually fixed it on pretty much the same day; however, I was in the middle of properly reimplementing the task-based asynchronous pattern for the library, so I was not able to push the commit and publish the NuGet package as the code was incomplete. I still have not finished but I think I will just publish a package solely to fix the reference to windows forms, and then I will update everything with a new package that has all of the fixes and new functionality that I added. |
|
Tested removing the NuGet reference to StandardStorage. Downloading your GitHub repo for the StandardStorage project and added that to the solution - worked like a charm. Tested on iOS.
|
|
@danibjor Thank you for your help; I will try and get this resolved sometime tomorrow. If you could please test that version as well when you have time, it would be greatly appreciate. To reiterate, I have not finished yet, but will soon. Thanks. |
…e-added support for AppVeyor through the use of OpenCover.
danibjor
reviewed
Jan 22, 2018
.appveyor.yml
Outdated
| build_script: | ||
| - msbuild /verbosity:quiet "Parse.sln" | ||
| test_script: | ||
| - .\Parse.Test\tools\OpenCover.4.6.519\tools\OpenCover.Console.exe -target:dotnet.exe -register:user -targetargs:""C:\projects\parse-sdk-dotnet\Parse.Test\bin\debug\Parse.Test.dll" /noshadow" -filter:"" -output:parse_sdk_dotnet_coverage.xml -oldstyle |
There was a problem hiding this comment.
targetargs => "c:" would probably not work so good on mac?
danibjor
reviewed
Jan 22, 2018
.appveyor.yml
Outdated
| - .\Parse.Test\tools\OpenCover.4.6.519\tools\OpenCover.Console.exe -target:dotnet.exe -register:user -targetargs:""C:\projects\parse-sdk-dotnet\Parse.Test\bin\debug\Parse.Test.dll" /noshadow" -filter:"" -output:parse_sdk_dotnet_coverage.xml -oldstyle | ||
| after_test: | ||
| - ps: | | ||
| $env:PATH = 'C:\msys64\usr\bin;' + $env:PATH |
There was a problem hiding this comment.
That's how the original was made.
There was a problem hiding this comment.
Also, this is just configuration for AppVeyor. No one will be running it locally.
|
@TheFanatr In regards to lack of reporting I would take a look at a log from the existing structure and compare it to the latest for this PR. Looking closely at the test section you can see a notable difference in the approach for running tests. The biggest one that sticks out to me is the choice between
and
I imagine once these are resolved we should be getting our reports again. You should probably run these commands directly in your local environment to inspect the behavior a bit more precisely, might help indicate why the reports are empty. Beyond that looking good now that you have CI working 👍 . Next step is to smooth out code coverage and report generation, then we can go from there. |
|
@montymxb I already saw all the reports and stuff, and have tested it locally. The problem is that if I use my local command arguments, it fails because it is unable to register the profiler as user for some reason. I have now tried both path32 and path64, and neither work. I will try to mess around with it some more when I have time. The reason I switched to VSTest is because I feel that it has better integration into Visual Studio but also because the API structure and naming scheme makes much more sense to me. |
|
@montymxb I found the issue. I was missing some of the files needed for OpenCover to work properly. I will push a fix very soon. |
|
|
Codecov Report
@@ Coverage Diff @@
## master #284 +/- ##
==========================================
- Coverage 67.21% 59.26% -7.95%
==========================================
Files 127 89 -38
Lines 10139 6258 -3881
Branches 1455 0 -1455
==========================================
- Hits 6815 3709 -3106
+ Misses 3127 2549 -578
+ Partials 197 0 -197
Continue to review full report at Codecov.
|
|
|
|
@danibjor Now that everything is configured, when you find a moment, could you please verify that the project is operational? |
|
The next steps are:
|
…ded NuGet package metadata to the Parse project because NuSpec file metadata declaration not supported for .NET Standard projects, added XML code documentation, and modified the AppVeyor configuration to upload the build-created NuGet package to the build console as an artifact.
|
Tried the latest sources on the master branch. Seems to be working fine on iOS so far - have done some CRUD-testing, but nothing heavy. Android is a whole new story. Tested on Android 7.1 device, with File-new-project, Xamarin.Forms, deploy. Success. Add the Parse project as part of the solution and reference to it. Trying to deploy on the device and it fails. App installs, starts up and fails. |
|
@danibjor That is super weird. |
|
As far as I can tell, this is due to the shared Mono runtime feature. A linker process reads through the app source and dependencies and gets rid of anything it thinks shouldn't be needed, then packages the leftover together, and puts all libraries and the app on top; it si possible that this process is removing that dependency erroneously. This has happened before with other projects according to my brief research. Try going to |
|
Also, what version of the Xamarin Android SDK are you using? .NET Standard 2.0 supports a minimum version of 8.0. |
|
@TheFanatr Tried a new Blank Android project: Adding just a reference to the nuget package, StandardStorage (v0.11) deploys fine. Adding the Parse project to the solution and adding a reference to my Android project, do not build. Complains about missing Microsoft.Extensions.PlatformAstractions in the Android project. Adding that nuget-package does not solve the prolem: This is really wierd, as it compiles fine when you add it to a mixed project wich targets both Android and iOS. thus, crash when it deploys and tries to start the App. The computer/dev env is up to date as of yesterday. |
|
Sidenote: When you do a file-new-project, with an Android only project, the highest .NET version you can choose is 4.6.1. No option to choose dotnet core. |
|
According to the compatibility table, .NET Standard 2.0 supports .NET Framework 4.6.1 as long as the Core 2.0 SDK is installed with it, and considering that it lists Xamarin Android 8.0 as supported, it should automatically add the .NET Core SDK. When creating new Xamarin projects, there is a template that lets you choose between using a PCL as the shared project or a .NET Standard Library as one instead. Could you please make sure that you are choosing the right one? Otherwise, what happens when you reference the |
|
Also, about .NET Framework 4.6.1 being the highest one you can target, you can install more targets here: https://www.microsoft.com/net/download/windows |
|
The error message above was from when I had the package added. Did not compile. I do not get the option to choose Dotnet core when I start a new android project. With the Xamarin forms project (iOS, Android, Win UWP), I choose dotnet core. |
|
core was up to date (v2.1.4). Updated .NET to .NET Framework 4.7.1 (latest release) - New test project with just Android. Same result. |
|
An extension ate my project files. This could take a while to clean up. When I get everything working again, I will try to get an Android app running and report back. |
|
Tried to build a test project on VS for Mac. After manually adding some Nuget packages (that is complained about was missing):
Tested some basic CRUD on iOS: looking good so far |
|
Oh, I think I know why that is happening. I will try to fix it soon. |
|
There's this extension that I was using called "EditorConfig Language Service" and it crashed Visual Studio about 10 times in a row, so I had to get rid of it, but now any project I call "Parse" will fail to open. I may have to reinstall Visual Studio. As soon as I can successfully open the project, I will start to tackle some of the issues. |
|
Update: I think that I found the source of the problem, and have used a workaround. I was in the middle of reformatting the source code and removing redundant interfaces, so the upcoming commit will include breaking changes but hopefully a fix as well. |
|
@TheFanatr Just give me a hint if you want me to test out something. I'm more than happy to spend time to get this SDK up and running with the latest bits of Xamarin. |
|
@danibjor Thanks. I was busy for the past day or so, but plan on getting back into fixing this up. Right now, I am in the probably nearing final stages of cleaning up redundant code, but I have to backtrack a little bit because I didn't realize that Moq really only works with interfaces, unless everything is virtual, which is not an option. |
|
Update: I have to redo everything. I should have ran the unit tests while I was making the changes; I'll know for next time. |
|
@montymxb In my investigation into how it would be possible to eliminate redundant interfaces within the codebase, I noticed that Moq, which is used in 87 of the unit tests, requires interfaces or at least classes with virtual members where mocked implementation is needed in order to actually set up the mocked objects. Knowing this, the only pheasible way I found that would allow for the removal of redundant interfaces without paying for a library like Typemock, is using Microsoft Fakes (https://docs.microsoft.com/en-us/visualstudio/test/isolating-code-under-test-with-microsoft-fakes). I think that, before anything else, I will try to convert all testing to the Microsoft Fakes system, in order to allow simplification of the codebase and easier development. Then, I will try to add more tests. Does this sound ok? |
|
I am also aware that the members that need to be mocked could be made virtual without much cost; however, it would also allow users to override those classes with their own implementations, and I don't think that is very safe potentially. |
|
@danibjor Could you please try the version that is currently in the repository? It may be working now that I have changed a few things. |
|
@TheFanatr In this repo? or your fork? |
|
Fork |
|
iOS, OK. Output when I try to deploy on an Android device |
|
@TheFanatr that sounds good to me, in regards to using msft fakes. Also wanted to note that we have access to the nuget package. When we're ready for an alpha we can set that up auto-package deployment as the final step of any release. |
|
So, after much testing, I have conluded that without using paid frameworks, it is impossible to fake non-virtual members. Microsoft Fakes only ships with paid versions of Visual Studio, and all the other options only have free trials. However, there is one option, though I don't know if it is worth it. In order to remove clutter, I could make it so that the interfaces only get compiled when everything is compiled with debug; we would be able to keep the current testing methodology, but remove the complication for end users. Until I recieve a response, I will be moving ahead with creating more unit tests and fixing ParsePush as well as Android support. |
|
Hmmm, if we can't work with msfakes reliable let's try and stay away from that then. It may require more work but in the meantime work on the unit tests and other components would make sense. |
|
Is there a general API spec? I am having trouble deciding how certain things with ParsePush should be implemented; is there a specification that should apply to every API? It was implemented slightly different on each of the platforms, and getting a generalized version is proving difficult because I don't know what is needed and what isn't. |
|
I see. I'll sit down when i get back and see where it was at if i can. It
should follow what the existing docs. However if this isn't accounted for
or out of date we should follow something similar to the existing
android/ios implementations.
Overall best to follow what the documentation leads devs to expect.
…On Tue, Feb 27, 2018, 17:51 Alex Fanat ***@***.***> wrote:
Is there a general API spec? I am having trouble deciding how certain
things with ParsePush should be implemented; is there a specification that
should apply to every API? It was implemented slightly different on each of
the platforms, and getting a generalized version is proving difficult.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#284 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE1d4FWglNALcVBCm7zleovaPrYbxdFJks5tZLE1gaJpZM4RWeT4>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request is what was discussed and previewed on #274. Essentially, the entire project is refactored into one .NET Standard assembly to improve maintainability and clean everything up. The next step is to re-modularize.