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

Fixes #74 #77

Merged
merged 5 commits into from
Aug 23, 2022
Merged

Fixes #74 #77

merged 5 commits into from
Aug 23, 2022

Conversation

jo-goro
Copy link
Contributor

@jo-goro jo-goro commented Aug 9, 2022

Fixes #74.

Two strongly typed ids yielded the same generated filename, e.g.:

namespace A { [StronglyTypedId] partial struct Id {} }
namespace B { [StronglyTypedId] partial struct Id {} }

Both yielded the same file Id.g.cs.

This commit uses the full name as the filename, yielding A.Id.g.cs and B.Id.g.cs for the previous example. Parentclasses are also included. Namespace, parent-names and id-name are seperated a dot. Should a subclass contain generic type parameters like

class A<TKey, TValue> { [StronglyTypedId] partial struct Id {} }

then the generated file name will be A__TKeyTValue.Id.g.cs. Two __ are used to sepereate the generic type parameters from the class name, since SourceProductionContext.AddSource(...) seems the throw an exception for any character other than [0-9a-zA-Z_\.]. Therefore, both

class A<TKey, TValue> { [StronglyTypedId] partial struct Id {} }
class A__TKeyTValue { [StronglyTypedId] partial struct Id {} }

would yield the same name. I don't think this limitation will cause many porblems, since most people won't have a classname like A__TKeyTValue. If anyone has a better idea I would be happy to implement it.

@andrewlock
Copy link
Owner

Thanks for this @jo-goro, I actually have a similar link sitting locally (though I prefer your implementation as it's far more thorough than mine is).

The trouble is, I'd like to add a unit test for this. I added a snapshot test like the following:

        [Fact]
        public Task CanGenerateMultipleIdsWithSameName()
        {
            // https://github.com/andrewlock/StronglyTypedId/issues/74
            const string input = @"using StronglyTypedIds;

namespace MyContracts.V1
{
    [StronglyTypedId]
    public partial struct MyId {}
}

namespace MyContracts.V2
{
    [StronglyTypedId]
    public partial struct MyId {}
}";
            // This only includes the last ID but that's good enough for this
            var (diagnostics, output) = TestHelpers.GetGeneratedOutput<StronglyTypedIdGenerator>(input);

            Assert.Empty(diagnostics);

            return Verifier.Verify(output)
                .UseDirectory("Snapshots");
        }

But the trouble is the test setup currently assumes that we only generate a single file, and I haven't found time to improve that yet. If you could add a regression test for the old/behaviour like the one above and fix the testing limitations that would be great, thanks!

@jo-goro
Copy link
Contributor Author

jo-goro commented Aug 15, 2022

Thanks for preferring my implementation, @andrewlock. I'll add the regression test and fix the current testing limitations.

@jo-goro
Copy link
Contributor Author

jo-goro commented Aug 23, 2022

@andrewlock I have added the regression test and changed the snapshot-tester to accomodate multiple generated Ids.

@andrewlock
Copy link
Owner

LGTM, thanks @jo-goro!

@andrewlock andrewlock merged commit 6d36325 into andrewlock:master Aug 23, 2022
lucasteles pushed a commit to lucasteles/StronglyTypedId that referenced this pull request Oct 24, 2022
* Fixes andrewlock#74

* Fixes whitespace

* Adds regression test

* TestHelper can handle multiple IDs

* Removes unused namespace
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.

Ids with the same name in the same project are not supported
2 participants