DB Connection Proposal impl, C# modules #1682
Conversation
lcodes
force-pushed
the
jeremie/module-api-cs
branch
from
a2895f7
to
dccfb37
Compare
|
This looks like at least an API break - is that correct? (If so, please add the |
| Product, | ||
| } | ||
|
|
||
| public static Dictionary<ISymbol, AnalyzedType> AnalyzeTypes(HashSet<ITypeSymbol> syms) |
There was a problem hiding this comment.
I think this one at least deserves some documentation; the idea is to process all types needed by table and reducer definitions at once (that includes all field types and all argument types), such that they only get instantiated once in the emitted output.
Initially, it was also driving the generation of AlgebraicType instances; but it's been decided to leave that for a later point in time and another PR. So this function still generates the info needed to emit table and reducer defs in the Module generator.
There was a problem hiding this comment.
This shouldn't be necessary; we already use structs for BSATN definitions, which are zero-cost to instantiate (they don't allocate or anything, they just use the default value on stack).
The only reason we store them in variables right now is for convenient aliasing in the codegen; other than that they could as well be static class and have the same performance cost in terms of instantiation upon use (aka none).
|
Like in the previous PR, this one seems to still contain a lot of unrelated changes (optimisations rather than DbContext implementation?). Are you still working on splitting it out? |
There was a problem hiding this comment.
It would be nice if table accessors were properties instead of methods. That would make them easier to filter in IDEs and also be more idiomatic to C#. Methods imply there may be some expensive computation under the hood, so I can see users trying to cache tables returned by those methods, which could lead to issues. Properties imply that data is readily available or inexpensive to obtain.
I know rust (and other languages) have methods, but as long as properties have the same names I'd consider it to be the same API, just sans parentheses. If I understand correctly, the goal of 0.12 is to have the same API across all languages, not to have code from one language be compilable in all other languages.
Also I acknowledge that this would've been best brought up during proposal review (or at least before work started). I wouldn't bring it up if it weren't API-breaking, so I'm sorry for the extra discussion / work
|
@SteveBoytsun FWIW I agree, but yeah, the proposal might be a better place for this. Since the proposal PR is still open, maybe raise the discussion there instead? Among languages we currently support, TypeScript would be in a similar position, so if we do want to use getters, we should probably at least use them in both C# and TS to make them consistent with each other. |
Yeah I wasn't sure where the right place for this discussion is. I'll leave a comment there, thanks! |
|
Moved to #1719 |
Refactor of impl following review, ended up simpler to work in a new branch.