-
Notifications
You must be signed in to change notification settings - Fork 81
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
[Discussion] Fundamental redesign of the library #102
Comments
Seems like it would really add a lot of flexibility, though I wonder how much it would affect one of the main pros of this library; is its simplicity. I might be missing something but being able to just add a package & attribute is much nicer than having to specify default templates if you're not going to need to use them. |
That's my biggest concern too. But one of the related issues currently, is that even you "only" add an attribute, you then have to choose one of (currently) 6 different backing types, followed by a factorial combination of (currently) 3 different interface implementations, and (currently) 6 different converters. That's something like 750 different potential combinations already 😅 And Pretty much every open issue wants to add another option...
So the goal is to still include default templates for all the current backing types, probably with most things implemented and converters like Json converters etc implemented, but commented out (as they require extra references). So essentially, the getting started experience should be the same - install the package (which adds the default templates), and add an attribute. And if you want to tweak the generated code, tweak the templates (or add new ones). The main "icky" thing, is that the package has to dump the templates into your project (so that you can customise them), which is a bit meh. There are other options:
|
I think this blurs the problem domain somewhat. You're moving away from a simple source generator that solves a specific use-case, to a more generic template-driven source generator which has many applications. I wonder if you could split it into two packages
That way consumers still get the simplicity of strongly-typed IDs using a standard set of base configurations, and then if they need something more, they can use the lower-level template-based generator: [StronglyTypedId]
public partial struct UserId { } vs. [TemplateSourceGenerator("strongly-typed-id-efcore")]
public partial struct UserId { } Your strongly-typed ID generator is akin to just inheritance: public sealed class StronglyTypedIdAttribute : TemplateSourceGeneratorAttribute
{
// Where @templates represents an embedded template?
StronglyTypedIdAttribute(string template = null) : base(template ?? "@templates/int") { }
} |
First of all, this library is great!! Adding this to an application is really simple, and that simplicity is really what I appreciate about this library. I agree with some of the others here, that requiring you to also add templates (and the possible maintenance burden with that) to your app would make it more difficult to use, and also convince team members to use this library. I understand that all the feature requests complicate matters a lot for this library, and I definitely agree with you that this all shouldn't be solved by this library. I'd argue that In short, I would also appreciate a more hybrid approach where this "user templating" would be added as a feature (more or less as @Antaris is suggesting), that gives an "escape hatch" for people that have "snowflake" demands, but still having the I'm here to help out, if needed. I have some experience with analyzers/codefixers, so that might come in handy. If there are any "up for grabs" issues, I'd be happy to help out with those, but I can imagine you want to hold that off for now, until you've made a decision on this design proposal. BTW Whatever you decide on this design, I'd back that up, as only you have the full overview of this maintainability problem. |
I'm tempted to say "what's the point of your source generator then?". Users should buy into your design decisions, or create their own. My instinct would be to allow users to customize the type of the underlying ID value, e.g.: [StronglyTypedId<Guid>()] // .net 7
[StronglyTypedId(typeof(Guid)] Perhaps have a constraint that it should be comparable. |
Thanks for your thoughts @Antaris, @tiesmaster and @egil!
I mean, you could use it like that I guess, but it would be pretty limited, and clearly focused on this one small use case. @mrpmorris's Moxy library is aiming to fill that niche. This would be similar, but more focused.
I'm noping out of that 😅 Definitely don't want to support 2 packages, I'll either carry on with the original approach or switch to the template one (likely with the extra niceties added later).
In case I wasn't clear, you wouldn't have to add templates, you'd only have to add them if you want to customise the output.
Trouble is, what's the "right thing"? I feel a bit like it's mostly already there, but there's certain things that are divisive, like impilcit/explicit conversions. There's no right or wrong answer. But adding all these extra options is what is going to bloat the library. I'm not sure where the right stopping point is, hence the template idea.
I wish it were only "snowflakes". My expectation is that basically everyone needs custom converters to use the IDs well, depending on whether you're using efcore, dapper, aspnetcore, swagger etc etc.
Yeah, I think that would give a nice on-ramp to templates
Creating a source generator is not trivial. Why use any library when you can create your own 😉🤷♂️
Yeah, it already does that, and that wouldn't change, it would just be done with templates instead. |
Thanks for the promotion :) |
Coming from twitter I too would like to express how awesome this library is 😄! Now, Personally I like this desing. I feel it could be even more easier to get started since all customization lives in the same attribute. I don't mind it being a small template engine since this would really allow me to implement my extra-features and most of the issues could definitely be turned into one more included template with the only problem being how do I name it. The I only thing that crossed my mind as I was reading the proposed desing is it would be great to allow multiple attributes or values so that you could have an additive effect on the generated code. eg: I decided I am using the Even better: If the templates could provide the I know the example is obviously not that strong for built-in things but for people that want to extend the functionality It could allow them to add to it without the need of editing & copying the templates and I do believe it would be easier to commit back new templates. No matter the decision I would still be happy to using the library since it is really awesome and it solves a small but great problem 😁 |
Thanks for the feedback @panoukos41 🙂
That really gets at the core of what I'm trying to achieve, so great to hear it echoed! Currently if I haven't implemented a feature you need, you fall off a cliff. With the templates, if you need an extra feature you can just add it (and optionally contribute a change to the templates back, which I'm much more likely to accept, as breaking changes pretty much go away)! 🎉
I love this idea - I would definitely allow providing multiple values to the attribute 👍 Getting very close to the Moxy mixin approach then 😉
It's a nice idea, though I'm not sure it would be feasible unfortunately. As an example, the converter for a |
These are always decorators on a class which add a property, aren't they? It's a shame we can't add attributes to existing members. I had to write a Fody weaver to achieve that goal. @andrewlock do you know you can already do this in Moxy? You could literally write one of the templates in a couple of minutes. I might give it a quick go when I next turn on my laptop:) |
@mrpmorris yep, totally get that, my idea is that this would essentially be a constrained subset of what Moxy aims to provide. Somewhat unrelated, but one thing I would kind of like to aim for in the templates is for them to be valid c# on their own, which would mean no moustache syntax. Although it's maybe not as essential for source generators as you get quick feedback anyway 🤔 |
I've created a short video showing how to make your own custom Strongly Typed Id classes in a matter of seconds simply by copy/pasting the code from this discussion and doing a quick Search/Replace. If the coder consuming the template wants it slightly different, they can edit the If you get an error in your mixin, VS will show it as a compiler error, telling you the filename of the mixin file, which line + column, and the compiler error message explaining why it won't compile. Give it a try, the source code is available here to try : https://github.com/mrpmorris/StronglyTypedIdMixins |
This is awesome. |
It is possible to have a StronglyTypedId based on the existing type instead of the text template? [StronglyTypedId<MyTemplateId>]
public partial struct UserId { }
readonly partial struct MyTemplateId : System.IComparable<Guid-or-whatever-type>, System.IEquatable<Guid-or-whatever-type> { ... } |
It seems that Moxy, the tool used, currently does not support structs, so we cannot use it :-) |
@markusschaber Fixed in 1.4 (just released) Thanks for letting me know! |
I like what @egil said regarding generics. I also really like simplicity of this library and do not expect it to solve every problem on Earth. The only thing I miss at the moment is the |
FYI, I (finally) got to the work for this in #117. I'm pretty sure this is the way I'm going to go, as it strikes the right balance between opinionated and flexible IMO. Plus it means I can close 30+ issues 😉 |
First of all, thanks everyone for your interest in this little source generator, as well as all the proposals for new features to include in the Ids! 🙏
All of the proposals I've seen, and the associated open issues have merit. The trouble is, adding support for all of them would add a huge array of options and flags to the library, making it more and more cumbersome to use, and adding an increasing burden to keep updating the library to add extra options. I don't think that outcome is sustainable or desirable.
Instead, I was considering rebuilding the source generator with a fundamentally different design. Instead of "building in" the definition of an ID into the source generator, consumers of the library would define the "templates" themselves (it would come with default templates matching the existing IDs). The
[StronglyTypedId]
then selects which template to use. For example, you would decorate your Ids something like this:You would add templates to your project by adding txt files with the name
StrongTypedId_<TEMPLATE>.txt
to the project. The source generator would read those, to build up a dictionary of templates, e.g.StrongTypedId_Int.txt
, "int" templateStrongTypedId_Guid.txt
, "guid" templateStrongTypedId_Guid-efcore.txt
, "guid-efcore" templateAn example of the template is shown below. The idea is that it's a "valid" csharp class, so it's easy to author them and then just change the extension. You would use
TESTID
(or similar) as a placeholder for the name of the ID:The source generator would use this template to generate the partial for each of the decorated IDs. It would take care of using the correct namespace, class hierarchy, and the correct ID names etc. by doing a basic replacement of
TESTID
forDefaultId
etc.To help people get started easily, the source generator could automatically copy the existing "basic" definitions for Guid/int/long/string IDs etc to a project. Consumers can edit these, delete them, or odd new ones as they see fit.
The big advantage is that it's easy for you to completely control how your IDs are generated in your own project, without needing to update the library. You can create as many different templates, as many as you need, for different purposes.
So... what do people think!?
This would obviously be a big change, but I think it will make the library much more flexible for everyone. I've written a PoC, and it all seems to work well, the question is, what do other people think?
Drop a 👍 if you think this sounds like a good idea, or 👎 if you don't. And feel free to drop me a comment!
The text was updated successfully, but these errors were encountered: