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

Generate using statements inside namespaces #803

Merged
merged 6 commits into from
Dec 2, 2019

Conversation

danlyons-home
Copy link
Contributor

This changes the stub generation's placement of using statements from the top of the file to inside the namespaces of each stub. It is a quick-and-dirty means to address name collisions seen in Issue #784

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)

Changed the code generation to place using statements for stub classes inside their respective namespaces

This commit fixes reactiveui#784
@dnfclas
Copy link

dnfclas commented Dec 2, 2019

CLA assistant check
All CLA requirements met.

using Xunit;
using Nustache;
using Nustache.Core;
using Refit; // InterfaceStubGenerator looks for this
Copy link
Member

Choose a reason for hiding this comment

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

Are the items in this file going to be skipped without this line? The generator looks for the Using Refit statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mea culpa - I have my editor set up to remove/sort using statements on save and I didn't notice the comment on that line. I'll re-add it.

An over-zealous remove/sort using task had removed it, but it is necessary for the tests to work
Including changes to generated stub files as a result
@clairernovotny
Copy link
Member

Looks like an easy fix that'll capture many of the namespace clashes without requiring use of semantic analysis, thanks!

Please add a few tests that show this working (and to prevent regressions) and I can merge it.

@danlyons-home
Copy link
Contributor Author

I added a simple test for the ClassTemplateInfo objects generated for a couple interefaces referencing SomeType from the CollisionA and CollisionB namespaces. Does that seem sufficient?

I could possibly add a test added to ensure the generated C# file compiles, but I'm not super familiar with Roslyn yet, and that may be too heavy-handed.

@clairernovotny
Copy link
Member

That test is a part of it. I think a test that shows it working is useful too....so force the generated code to compensate for having two different SomeType instances in different namespaces, so we can prove the generated code works as intended.

I don't think you need to actually compile it as compiling the unit test project should trigger that code path.

Renamed ITypeCollisionApiB's method to SomeBRequest for consistency
Updated generator tests to include the Refit namespace
@clairernovotny clairernovotny merged commit 6282a29 into reactiveui:master Dec 2, 2019
@clairernovotny
Copy link
Member

Thanks!

@lock lock bot locked and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants