customtype: add ability to register any custom type #27
Conversation
customtype/customtype.go
Outdated
|
|
||
| // Is indicates if the given reflect.Type has a custom AST representation | ||
| // generator registered. | ||
| func Is(rt reflect.Type) (func(any) ast.Expr, bool) { |
There was a problem hiding this comment.
How about keeping this in the same top-level valast package? e.g. valast.RegisterType, valast.IsType - that seems nicer to me
There was a problem hiding this comment.
I'm not sure valast.IsType really works because this only applies to a super narrow subset of things right now 🤔 Additionally a subpackage keeps the scope of the access to the types map clear
But, WDYT about adding valast.RegisterType as an alias for customtype.RegisterType? I've seen this used in various libraries for exposing some internal stuff to consumers
|
looks great, I only have one minor comment but otherwise happy with that approach. Let me know when you update & want this merged/a new version released. |
|
|
||
| t, ok := customTypes[rt] | ||
| return t, ok | ||
| } |
There was a problem hiding this comment.
sorry @bobheadxi I think I should've been more clear - my point is I'd rather not have a tiny 35-line one-file package and would prefer this just lives in the root under a customtype.go file
does that make sense?
There was a problem hiding this comment.
I mentioned some reasoning in #27 (comment) - it makes the interface to the rest of the valast codebase clear, and avoids adding globals to the root package internally. IMO that's a strong reason to make this a separate subpackage
#63959) This change follows https://github.com/sourcegraph/sourcegraph/pull/63858 by making the _all_ subscriptions APIs read and write to the Enterprise Portal database, instead of dotcomdb, using the data that we sync from dotcomdb into Enterprise Portal. With this PR, all initially proposed subscriptions APIs are at least partially implemented. Uses hexops/valast#27 for custom `autogold` rendering of `utctime.Time` Closes https://linear.app/sourcegraph/issue/CORE-156 Part of https://linear.app/sourcegraph/issue/CORE-158 ## Test plan - [x] Unit tests on API level - [x] Adapters unit testing - [x] Simple E2E test: https://github.com/sourcegraph/sourcegraph/pull/64057
This change extends #25 to allow any type to be configured with a custom AST representation, and demonstrates this by migrating the existing
time.Timehandling to use this - seestdtypes.gofor example. The existingtime.Timeasserts that the functionality remains unchanged.This is useful for any type in the same situation as
time.Time, including type aliases oftime.TimeThis could be useful for #21 and #11, allowing users to bring their own representations of types, though it currently does not allow you to override types that are already registered.