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

.NET Core support for Castle.Core #92

Closed
wants to merge 5 commits into from

Conversation

MattGal
Copy link

@MattGal MattGal commented Jun 16, 2015

Port Castle.Core to .NET Core profile

  • Previously raised and known issues have been addressed
  • (Outdated) Xunit tests are passing with some disabled :
  • Tests which used Assembly path-based loading APIs are disabled as this is not explicitly provided in .NET Core (Can be achieved via dependency injection in ASP.NET 5-hosted apps but it doesn't seem appropriate to depend on this. Please comment if not, as I can provide implement it taking a dependency on ASP.NET 5 interfaces.
  • Five DictionaryAdapterFactory tests have been marked as Skip:
    • Two depend on ICustomAttributeInfo interface that has been removed in the Core profile. I continue to investigate ways to make this work.
    • Three are failing because code like this correctly applies the prefix attribute, but fails to correctly add it to the IDictionary accessor.

        var person = factory.GetAdapter<IPersonWithPrefix>(dictionary);

        person.Name = "Craig";

        Assert.Equal("Craig", dictionary["Name"]);
  • Commits have been squashed and are currently mergable with no conflicts
  • Aside from the DictionaryAdapterFactory issues I am done with associated work and awaiting for guidance on how to move forward
  • I am available post-merge to assist with any follow-up needs including coordinating with teams at MS, fixing issues, etcetera, for as long as this is needed. Please make sure to include @MattGal on posts so I get alerted.

Thanks everyone for your patience, I'm eager to get this right and merged.

(Edited from previous WIP state)

@richlander richlander mentioned this pull request Jun 16, 2015
@MattGal MattGal force-pushed the Add-NetCore-Beta5 branch from 98adb3c to 75a1867 Compare June 24, 2015 18:30
@uhaciogullari
Copy link

I think you had a problem with line endings.

@MattGal MattGal force-pushed the Add-NetCore-Beta5 branch 2 times, most recently from 6146b77 to 1700a59 Compare June 26, 2015 00:57
@kkozmic
Copy link
Contributor

kkozmic commented Jun 26, 2015

Nice starting point @MattGal thanks for taking that on.

It looks like there is some issue with code indention that makes out quote hard to read through the changes and have a meaningful conversation about them.

@floydpink
Copy link

Adding ?w=1 as mentioned under the Whitespace section here would allow reading through all that indentation noise, @kkozmic - https://github.com/castleproject/Core/pull/92/files?w=1

But I agree that getting the same indentation settings into the editor would be a great idea.

On that note, the .editorconfig probably could be beefed up a little more to enforce some more of the project coding-style.

Great work, @MattGal ! 👏

@kkozmic
Copy link
Contributor

kkozmic commented Jun 26, 2015

Sounds like a pull request Hari 😉 (editorconfig)

On Fri, 26 Jun 2015 12:42 Hari Menon notifications@github.com wrote:

Adding ?w=1 as mentioned under the Whitespace section here
https://github.com/blog/967-github-secrets would allow reading through
all that indentation noise, @kkozmic https://github.com/kkozmic -
https://github.com/castleproject/Core/pull/92/files?w=1

But I agree that getting the same indentation settings into the editor
would be a great idea.

On that note, the .editorconfig
https://github.com/castleproject/Core/blob/master/.editorconfig
probably could be beefed up a little more
http://editorconfig.org/#example-file to enforce some more of the
project coding-style.

Great work, @MattGal https://github.com/MattGal ! [image: 👏]


Reply to this email directly or view it on GitHub
#92 (comment).

@floydpink
Copy link

@kkozmic - I shall try and get a PR in with an updated .editorconfig

@MattGal - the project's coding standards does suggest using only tabs and not spaces. It also talks about how ReSharper (if you use it) can be configured to use the Shared project settings.
Also, if you do not have it yet, do you want to try installing the EditorConfig VS Plugin and see if that helps to enforce tabs?

@MattGal MattGal force-pushed the Add-NetCore-Beta5 branch 2 times, most recently from bbc4deb to af80161 Compare June 29, 2015 19:13
@jonorossi
Copy link
Member

@MattGal that actually made it worse. Unfortunately we don't have a completely consistent repository and it looks like you've run a formatter over the whole thing which introduced heaps more whitespace changes. For now I'd just leave this PR as is with respect to my comments in #90.

@MattGal MattGal force-pushed the Add-NetCore-Beta5 branch from 43361d3 to a6f7d46 Compare July 6, 2015 20:25
@uhaciogullari
Copy link

@MattGal Just an idea: you can just exclude DictionaryAdapter folder in project.json so you wouldn't need to modify all those files.

@MattGal
Copy link
Author

MattGal commented Jul 7, 2015

@uhaciogullari , I had thought of that, but there is a fair amount of functionality in the folder that does actually build and may end up being useful. Also, the tax is paid now :)

@richlander
Copy link

Anything left for @MattGal to do to land the PR?

@jonorossi
Copy link
Member

I don't want to sound rude or ungrateful for the effort but there is still quite a bit of work to do. We and our users expect the same sort of level quality as the .NET Framework does, i.e. this can't just be a fire and forget port to .NET Core. If we are changing code there needs to be a reason for it, and that needs to be documented so we can continue maintaining this code as we have been for well over a decade.

From a cursory look, things to do include:

  • There are now quite a few added #ifdefs that have code in .NET Core but nothing in everything else (i.e. everything else won't compile)
  • Fix the TODOs that refer to the fact that the correctness needs to be verified
  • Fix or document why we need some of the changes which looks like the code now doesn't do what it does on all the other platforms. Some where I assume the API or overload may not be available in .NET Core that the code just ignores parameters, passes null, or doesn't do certain checks.
  • Unit tests
  • Determine whether dnxcore50 is the right framework, should we be using dotnet?
  • Consider using a symbol other than CORECLR for .NET Core because it really isn't specific to the CoreCLR, if anything it would be COREFX, DNX, NETCORE, ....
  • Look at what DNX generates as a NuGet package and how that compares to our build scripts' output, since I assume we'll be using that from now on

@MattGal MattGal force-pushed the Add-NetCore-Beta5 branch 3 times, most recently from 0345944 to 3f42ddb Compare July 28, 2015 00:06
@MattGal MattGal force-pushed the Add-NetCore-Beta5 branch 3 times, most recently from 13c1289 to 9a13cd2 Compare July 30, 2015 22:10
@MattGal MattGal force-pushed the Add-NetCore-Beta5 branch from 9a13cd2 to 299bd7d Compare July 31, 2015 18:40
@MattGal MattGal changed the title [WIP] : .NET Core support for Castle.Core .NET Core support for Castle.Core Jul 31, 2015
@MattGal
Copy link
Author

MattGal commented Aug 12, 2015

@jonorossi , any update on this, or other things I can do to improve the PR?

@adamralph
Copy link

As can be seen from the issue links, the three major OSS .NET mocking frameworks (Moq, NSubstitute and FakeItEasy) are all under pressure from the community to provide support for .NET core so there is a lot of interest in getting this in. All efforts are hugely appreciated.

@AmadeusW
Copy link

I'd like to help getting this done. What's left to be done from @jonorossi previous comment? And how is this PR related to the netcore branch?

@jchannon
Copy link

jchannon commented Jan 4, 2016

What's the status of this? We want to get Nancy on CoreCLR which depends on FakeItEasy which depends on this 😄

@thecodejunkie
Copy link

I'll +1 this. Like @adamralph said, it's a core (no pun intended) component for the major mocking frameworks used by .NET OSS projects

@jonorossi jonorossi closed this Jan 5, 2016
@adamralph
Copy link

So what's the status? Has this been done elsewhere?

@jonorossi
Copy link
Member

See the linked issue, #90.

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.