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

Reconsider topic names as variable names #1919

Closed
matt2e opened this issue Jul 1, 2024 · 8 comments · Fixed by #1958
Closed

Reconsider topic names as variable names #1919

matt2e opened this issue Jul 1, 2024 · 8 comments · Fixed by #1958
Assignees
Labels

Comments

@matt2e
Copy link
Collaborator

matt2e commented Jul 1, 2024

When defining a topic, this is the code:
var VarName = ftl.Topic[EventType]("name")
When we export a topic, this is the code we generate
var Name = ftl.Topic[EventType]("name")
I think this breaks the mental model people have when switching between modules as the variable they exported in one module is not the name to access it in another.

Also:

  • We need to check for name collisions wit the var name with other decls
  • We need to make sure the var name starts with a capital letter
@github-actions github-actions bot added the triage Issue needs triaging label Jul 1, 2024
@ftl-robot ftl-robot mentioned this issue Jul 1, 2024
@matt2e
Copy link
Collaborator Author

matt2e commented Jul 1, 2024

Add an error when name and varname are different. Error should explain why.

@matt2e matt2e added next Work that will be be picked up next and removed triage Issue needs triaging labels Jul 1, 2024
@matt2e
Copy link
Collaborator Author

matt2e commented Jul 1, 2024

String should be translated to var name capitalization. ie var name changes per language (idiomatic)

@matt2e
Copy link
Collaborator Author

matt2e commented Jul 1, 2024

Do the same for name string literal? Think about it

@matt2e matt2e added the P1 label Jul 1, 2024
@safeer safeer self-assigned this Jul 3, 2024
@github-actions github-actions bot removed the next Work that will be be picked up next label Jul 3, 2024
@safeer
Copy link
Contributor

safeer commented Jul 12, 2024

Currently, given a topic declaration:
var publicBroadcastTopic = ftl.Topic[EventType]("public_broadcast")
The generated code will be:
var Public_broadcast = ftl.Topic[EventType]("public_broadcast")

The issue is if we generate the idiomatic variable name var PublicBroadcast, we lose the ability to deterministically derive the topic name from the generated variable name. I.e. is the topic "public_broadcast" or "publicBroadcast"?

The options I'm seeing are either to constrain the topic names (say, to lower snake), which feels too prescriptive, or to just enforce that the varname be the same as the topic name with the first letter capitalized (var Public_broadcast). The latter is gross, but it does align the original code with what's being currently generated.

cc @matt2e @wesbillman

@safeer
Copy link
Contributor

safeer commented Jul 12, 2024

Actually, unless I'm missing something, I don't think we do ever go from generated variable name back to topic name, despite what this comment implies:
https://github.com/TBD54566975/ftl/blob/58b22ff793a850a66de471b31bbb61e3b2ff0235/backend/schema/validate.go#L345

Which means for the go generator template, we could use the topic "public_broadcast" to generate var PublicBroadcast and enforce that during compile time as well. Gets rid of the grossness and aligns the source with the schema-generated code.

Just need to add a new string conversion func for public_broadcast -> PublicBroadcast to use in the generator and analyzer

@safeer
Copy link
Contributor

safeer commented Jul 12, 2024

Actually, unless I'm missing something, I don't think we do ever go from generated variable name back to topic name, despite what this comment implies:

Aaand I might be mistaken. Not sure what the clean solution is here.

@matt2e
Copy link
Collaborator Author

matt2e commented Jul 15, 2024

I honestly don't know a good solution to this.

I'm tempted to go down the path of a single allowed format for the topic name string... That way we can deterministically transform from the var name to it.

Or we figure out a way to make the compiling work where we never need to derive the topic name from the variable name. I guess you could do this by doing a first pass where you collect all the variable names + topic name pairs, and then refer to that mapping whenever we find a topic variable used. Then it doesn't matter that variable name loses information from the topic name.

Maybe that second one is best?

Throwing out some other options that probably aren't as good...

  • you pass in a list of strings like ftl.topic[Event]("public", "broadcast") and error if theres any camel casing, or underlines
  • we transform topic names behind the scenes such that "public_broadcast" == "publicBroadcast" because their normalized forms are the same

@safeer
Copy link
Contributor

safeer commented Jul 16, 2024

Went down the path of not deriving the topic name by instead looking it up in the ast during analysis, and potentially storing the varname to topic name mapping as an analysis fact, but another issue we discovered (thanks @worstell!) is that the analysis isn't done on the stubbed code. Expanding the analysis to include that isn't trivial; it might make sense if we need that info for anything else, but doesn't seem worth it for this issue.

I'm also leaning towards being prescriptive about topic names (let's say lower_snake) so we can deterministically transform in either direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants