Skip to content

feature: bring reactiveui.fody into the main project#1503

Merged
vatsalyagoel merged 11 commits intomasterfrom
fody
Jun 10, 2018
Merged

feature: bring reactiveui.fody into the main project#1503
vatsalyagoel merged 11 commits intomasterfrom
fody

Conversation

@ghuntley
Copy link
Member

@ghuntley ghuntley commented Oct 6, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature

What is the current behavior? (You can also link to an open issue here)

@kswoll has been doing a stunning job over the last couple years maintaining his addin. I think it's high time to in-line his work and make it official. We had conversations back in May - this PR just makes it official and does the needed to make it happen.

What is the new behavior (if this is a feature change)?

We are in-lining https://github.com/kswoll/ReactiveUI.Fody and making it an official thing. Kirk Woll will get commit bit access to the reactiveui/reactiveui repository as a contributor/maintainer so that he can still work autonomously but over time the core team will also increase their knowledge of things they maintain to include fody.

What might this PR break?

Nothing

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:

  • We need @kswoll to be the person that presses the merge button.
  • This PR still needs work before merging.

<<<<<<< HEAD
MethodDefinition[] getMethods;
if (property.SetMethod == null && property.GetMethod.TryGetMethodDependencies(out getMethods))
=======
Copy link
Member Author

Choose a reason for hiding this comment

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

dis needs fixing

});
<<<<<<< HEAD

=======
Copy link
Member Author

Choose a reason for hiding this comment

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

dis too

@kswoll
Copy link

kswoll commented Oct 7, 2017

Hello all! Catching up on this, but excited to see this all get merged. Currently ReactiveUI.Fody is using AppVeyor to build and publish to Nuget. How do you see this happening going forward? What is the best strategy? Will this get incorporated into the regular ReactiveUI build process? Should I add one or more people as owners to the nuget package?

@ghuntley
Copy link
Member Author

ghuntley commented Oct 7, 2017

Okay so @kirkplangrid you'll no-longer be the single person maintaining this, the software and yourself will inherit a team of maintainers. The addin is now considered part of ReactiveUI as a releasable unit -
if Fody breaks then the build breaks etc. Additionally you'll no longer need to run your own CI pipeline or any of that awesome stuff that often ends up being a timesuck from doing software development. The biggest change will be that Fody's version number will match the ReactiveUI version number and will be pinned to only work with that particular release (this is a very good thing from a support POV!) as it is now considered a component of ReactiveUI that's built on every build.

I need you to add add the NuGet.org users "@ghuntley" and "@reactiveui" as owners to the NuGet package. Thx 👍

@ghuntley
Copy link
Member Author

ghuntley commented Oct 7, 2017

Wooot - thank-you. Can confirm that NuGet is setup appropriately. Need to clean up the PR a little bit and tweak build.cake then we can ship v8 compatible version.

image

ghuntley added a commit to ghuntley/ReactiveUI.Fody that referenced this pull request Oct 8, 2017
@ghuntley
Copy link
Member Author

ghuntley commented Oct 8, 2017

@kswoll A label has been created especially for reactiveui-fody

image

You now have maintainer access to do things like merge pull-requests, triage labels etc. Please drop on by Slack and speak with myself before doing your first couple of PRs and I'll show you the ropes. Some things are a little different such as:

  • ReactiveUI uses a squash+rebase workflow that combines all commits into a single commit and at that moment of time just before you press the green button you must reword the commit message to match the project convention.
  • Our GitHub issues are automatically closed after 67 days of inactivity unless the maintainer tags the issue with an exemption. https://github.com/reactiveui/ReactiveUI/blob/develop/.github/stale.yml#L8

A PR has been sent over back upstream to your repository that directs people to this discussion and informs them that the project has moved.

kswoll/ReactiveUI.Fody#51

@ghuntley ghuntley force-pushed the fody branch 2 times, most recently from 71fe59f to 8a62674 Compare October 9, 2017 11:08
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.487% when pulling 8a62674 on fody into 08d9e58 on develop.

@bradphelan
Copy link
Contributor

Anything blocking this pull request. I've just discovered that my upgrade to ReactiveUI.Fody to support 8.0 doesn't actually work.

@ghuntley
Copy link
Member Author

ghuntley commented Oct 11, 2017

The random errors I was getting with the toolchain 'cause we are doing things that aren't implemented in the visual studio editor (but are implemented in the compiler). Oren pushed a fix last night so work can proceed. It is not in a mergeable state atm.

@wmcainsh
Copy link

Are there any workarounds for this atm? I'm trying to update another nuget package that has a dependency on 8.0.

@worldbeater
Copy link
Contributor

worldbeater commented Jan 19, 2018

Curious if ReactiveUI.Fody in this repo is supposed to work. Is this branch open for further pull requests? What is this project's roadmap, will ReactiveUI.Fody play well with .NETStandard only or are you going to support other targets as well? I wish I could use this with ReactiveUI v8 on NETStandard 1.3, but the ReactiveUI.Fody NuGet package doesn't seem to support it.

@ghuntley ghuntley requested review from a team May 15, 2018 01:23
<PackageReference Include="xunit.runner.console" Version="2.3.1" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
<PackageReference Include="Xunit.StaFact" Version="0.2.9" />
<PackageReference Include="Nerdbank.GitVersioning" Version="2.1.23" PrivateAssets="all" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be in the IsTestProject item group. Otherwise, versioning doesn't seem work in the branch.

@gtbuchanan
Copy link
Contributor

I think the only thing remaining here is to fix the versioning and the NuGet package format. I'm pretty sure the netstandard ReactiveUI.Fody.dll should be in the package root for Fody to actually pick it up. Because of this, I also think netstandard is the only TargetFramework required for the ReactiveUI.Fody project. Additionally, ReactiveUI.Fody should probably have a dependency on ReactiveUI.Fody.Helpers since it won't work without it.

I know how to fix the versioning and add the reference, but I'm not familiar enough with the new csproj/NuGet to know how to drop the files in the root of the package instead of the lib folder. This is how the current ReactiveUI.Fody package looks:

image

@vatsalyagoel
Copy link
Member

How can I help?

* Fix package versioning

* Fix ReactiveUI.Fody NuGet package and include tests in build

* Add Fody projects to solution
Copy link
Member

@vatsalyagoel vatsalyagoel left a comment

Choose a reason for hiding this comment

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

Tested the changes locally. The package works great

@vatsalyagoel vatsalyagoel changed the title [WIP] feature: bring reactiveui.fody into the main project feature: bring reactiveui.fody into the main project Jun 9, 2018
@vatsalyagoel vatsalyagoel changed the title feature: bring reactiveui.fody into the main project [WIP] feature: bring reactiveui.fody into the main project Jun 9, 2018
@vatsalyagoel
Copy link
Member

I think the only part remaining is updating the docs to add Fody

@vatsalyagoel vatsalyagoel changed the title [WIP] feature: bring reactiveui.fody into the main project feature: bring reactiveui.fody into the main project Jun 10, 2018
@qrzychu
Copy link

qrzychu commented Aug 13, 2018

When and how can we use this?
Today, I added Fody package, added in FodyWeavers.xml and it cannot find the weaver. Is this how this will work?

Cannot wait till it's done

@gtbuchanan
Copy link
Contributor

@qrzychu This was released a couple months ago. You need to reference the newer ReactiveUI.Fody package and add <ReactiveUI /> to FodyWeavers.xml. If you have problems, please turn to StackOverflow or create a new issue with more details. This is already confirmed working by many people.

glennawatson pushed a commit that referenced this pull request Mar 23, 2019
* added fody

* Fix unhandled exception on netstandard (#1578)

* Align Fody dependencies to ReactiveUI (#1649)

* added @reactiveui/fody-team

* Finish up ReactiveUI.Fody (#1671)

* Fix package versioning

* Fix ReactiveUI.Fody NuGet package and include tests in build

* Add Fody projects to solution

* Fixed CLI build for Fody

* Added another Reactive Test
@lock lock bot locked and limited conversation to collaborators Jun 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.