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

Update HomoIcon tool to reflect new repository structure and manual code changes #537

Merged
merged 31 commits into from
Jun 1, 2018

Conversation

quinmars
Copy link
Contributor

This is a renewed version of PR #474.

The orignal text was:

Originally, I started to work on issue #420 but I than realized that the HomoIcon tool does not work in its current form. The paths are not setup right and the formerly generated files seem to be hand optimzed (many #ifdefs are removed, the type notation in the docs was changed etc.).

This PR tries to fix those problems. The paths are valid again. Several #ifdefs are removed. From a code point of view it is almost the same (some usings differ and some trailing white space was removed). While I tried to reflect the changes in the documentation as well, there are some type notations that differ (and it is only the notation that differs not the type). I have no idea if there is an easy way to fix those:

-     /// <seealso cref="Observable.ToEventPattern{TEventArgs}(IObservable{EventPattern{TEventArgs}})" />
+     /// <seealso cref="M:System.Reactive.Linq.Observable.ToEventPattern``1(System.IObservable{System.Reactive.EventPattern{``0}})" />

@bartdesmet, maybe you can tell which tool you used.
But I don't think it is such important, since the generated xml file won't differ.

Note: I haven't commit the newly generated files yet.

@quinmars quinmars changed the title Update homoicon Update HomoIcon tool to reflect new repository structure and manual code changes May 26, 2018
@@ -5,6 +5,9 @@
<PackageTags>Rx;Reactive;Extensions;Observable;LINQ;Events</PackageTags>
<Description>Reactive Extensions (Rx) for .NET</Description>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Debug|net46|AnyCPU'">
<DocumentationFile>bin\Debug\net46\System.Reactive.xml</DocumentationFile>
</PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this property group, it shouldn't be needed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. It is however useful to catch wrong documentation. ☺️

@danielcweber
Copy link
Collaborator

@quinmars I'm not really into this program but if it does the job I'll happily merge it, however, there's already some conflicts due to the latest changes on master.

@quinmars
Copy link
Contributor Author

@danielcweber, I also don't know every detail of that program. I just applied some changes, so that the generation works again and matches as nearly as possible the hand-edit formerly generated files. Since there is a unit test, which compares the public surface of the Observable and the Qbservable class, it shouldn't be harmful as long as the test pass.

I'm planning to write a little readme file that explains when and how to use the HomoIcon tool.

@danielcweber
Copy link
Collaborator

Awesome, the approval tests already kicked in here! @ryanwersal can you have a look at it?

@ryanwersal
Copy link
Contributor

@danielcweber I'd be happy to take a look later today as well as ensuring VSTS sees the approval tests.

@ryanwersal
Copy link
Contributor

It looks like the approval tests are failing for a valid reason but unfortunately aren't finding a good way to report it while running under VSTS. I'm currently investigating that. In the meantime, there are a few API changes to consider. For convenience, I added the diff to a gist. Most appear to be lines moving around but there are a couple signature changes.

I'm going to see if I can get it so that such a diff is produced by the failing test while building in VSTS instead of having to retrieve it out of band.

@quinmars
Copy link
Contributor Author

@ryanwersal thanks alot, It's the Action parameter of AutoConnect that is not wrapped into an expression. I actually have no idea how that will or should work in the context of Qbservables. An serverside Action does not make much sense. On the other hand I'm unsure if the current expressionless implementation will work. I'll take a closer look on it tomorrow.

@quinmars
Copy link
Contributor Author

Ok, I watched now Bart's video "Observations on IQbservable - The Dual of IQueryable" at channel 9.

I understood it that way, that every method of Observable that does not have a direct homoiconic form, i.e., the method does not have IObservable as first argument, starts with an IQueryProvider argument. That makes it possible to for example Range remotely.

    IQbservable<int> = myprovider.Range(0, 10);

Now, I still haven't any clue, how Qbservable.AutoConnect could be useful, but there is the precedence of Qbservable.RefCount so it would be odd to exclude AutoConnect. Given that all other lambda's are used as Expression<>s, I personally think it would be more correct or at least more consequent to also have it that way for AutoConnect. The query provider can still compile it, if it needs the Action locally.

So there a two possibilities:

  1. Keep the Qbservable.AutoConnect signature as is.
  2. Change it to an expressional form.

In the first case I would move AutoConnect to the non-generated file and exclude it from the homoicon tool, because special casing inside of the tool doesn't look to be reasonable. The second option would be an API break. But I don't think anyone is currently using that method. Personally I prefer option two, because it is more in line with the preceding operators.

Maybe @RxDave, the author of Qactive, has a more profound opinion on that. Qactive is the only serious IQbservable provider implementation I have found so far.

@quinmars
Copy link
Contributor Author

I just realized that AutoConnect is not part of the 4.0 release. It actually would not be an API breakage to choose option 2. Are there any arguments to keep Qbservable.AutoConnect in its current form?

@quinmars
Copy link
Contributor Author

@ryanwersal One problem is indeed that the test is sensitive to the order the members. The generator does sort the members by their name, but that's not sufficient for methods with overloads. Some how the argument types must be considered.

@quinmars
Copy link
Contributor Author

quinmars commented Jun 1, 2018

Ok, I integrated now #562 and chosed option 2. Finally I reordered some definitions in the approved API file. It should now pass all checks.

@danielcweber danielcweber merged commit 8ebfb49 into dotnet:master Jun 1, 2018
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.

5 participants