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

How we get input declarations in the AST as devs #74

Open
NilsJacobsen opened this issue Jun 6, 2024 — with Linear · 24 comments
Open

How we get input declarations in the AST as devs #74

NilsJacobsen opened this issue Jun 6, 2024 — with Linear · 24 comments

Comments

Copy link
Member

When developers add variables to a m-function they might think that this input declaration is also available in the ast (aka in the project for all other apps). But that is not the case.

Example

Developer knows that there is a count variable in the ref language so he adds the count variable right to the m function. At this point when I open fink, there is no count available as an input declariation in the ast. Im basically not able to use count as referencePattern or a selector unless I added the input again manually.

Proposal 1: Add a lint rule that is resolvable.

When I argument is passed into a m-function without a input declaration definition in the ast it get's linted. You can auto resolve the lint by adding it to the ast.

Proposal 2: Add a vs-code shortcut

Similar to the extract mechanism we could also add the input declaration in the ast and in the code with one step.

I fell like the lint rule would be the smoother solution, but I don't know how long we need to wait for it. This feature is needed at the time of message bundle component rollout, because this is such a basic use case.

@NilsJacobsen NilsJacobsen changed the title How we get input declarations in the AST How we get input declarations in the AST as devs Jun 6, 2024
Copy link
Contributor

You mean how to extract this in Sherlock?

I have two donuts. 🍩 = m.donut_count({count: 2})

Copy link
Member

Yep. Sherlock must have a way to extract declarations too/have a keyboard navigation thingy for devs to do it fast

Copy link
Contributor

felixhaeberle commented Jun 6, 2024

I disagree on that.

A static string is a normal string without any declaration. This string can later be made to something more through editing and message function can then also consume variables through props, but specify that at "message create/extract" goes against our "first simple message" approach where we add complexity gradually through the message component and not upfront.

Copy link
Member

@felix.haeberle code will have this

<p>You have {numberPhotos} photos</p>

you need to extract {numberPhotos} in sherlock as a declaration

Copy link
Contributor

felixhaeberle commented Jun 6, 2024

You can do this in the message component? Also its not that easy. You now also have to specify whether it is photo/photos in this string for example. You have to jump in the message component either way for that.

Copy link
Contributor

At first, I would default to "edit in message component", and only after people request more VS Code built-in functionality: build it.

Copy link
Member

that means that @nils.jacobsen needs to build a "extract declarations" feature into the message component

Copy link
Member

i leave it up to nils on what to do

Copy link
Member Author

Scenarios:

  • Declaration is written in extractable message, get's lost when extracted. ⚡️ (not in ast, not configurable in message bundle component)
  • Dev wants to add a new declaration to m function ⚡️ this is not possible (it doesn't get in the ast, their expectation will be: it's passed in, why doesn't it work?)
  • Auto extraction would cause problems if it is not capable of doing this ⚡️
  • Declaration added in ui first (fink, sherlock) m function gets linted ✅

    Problem: Without such a helper mechanic, devs always need to add a input declaration in the ui and then add the variable in code. (2 places where to add, which is not optimal)

Copy link
Contributor

felixhaeberle commented Jun 6, 2024

that means that @nils.jacobsen needs to build a "extract declarations" feature into the message component

Not extract, why extract? Just create a declaration and use it in the message. Please don't overcomplicate this UX with users who have complete different setup where it gets really hard to do this (extract) right across librarier and potentially across languages.

  • Declaration is written in extractable message, get's lost when extracted.

Expected, how should Sherlock know?

  • Dev wants to add a new declaration to m function ⚡️ this is not possible (it doesn't get in the ast, their expectation will be: it's passed in, why doesn't it work?)

Same as above

  • Auto extraction would cause problems if it is not capable of doing this

No problem as it is just not supported and you have to go through it afterwards.

I still don't get why we should overload the complexity of a simple text extract / message create when this is expected to be gradually added and not upfront.

@nils.jacobsen Potentially a LOOM could help to understand your requirement more, or a small session in Around.

Copy link
Member

@nils.jacobsen correct me if my loom is wrong. we want 3 things:

  1. sherlock ideally extracts a message with correct declarations
  2. we want a lint rule that detects "no declaration but variable reference used"
  3. the message editor component ideally has a "extract as declaration" feature

https://www.loom.com/share/a73d0144b4324fdaa559a15dd568a686?sid=6a2f2868-77e3-447b-b3db-861183b3dbd1

Copy link
Member Author

Small addition on the loom:

  • we probably a resolvable lint rule that adds a argument into the ast
  • we might want a meanwhile solution if the problem gets popular

https://www.loom.com/share/78f08810532b4b0996f2669a27d4897c?sid=1dae3b31-478c-4ed1-957b-0dcd906c2010

Copy link
Contributor

felixhaeberle commented Jun 7, 2024

@samuel.stroschein @nils.jacobsen Okay folks we are mixing quite a few things here & have reached a scope way above Sherlock. Let me summarize this discussion, we need:

Parser

  1. A variable reference parser which parsers strings in multiple formats & potentially multiple languages to be used by Sherlock when extracting strings and a potential lint rule to detect variable usages in existing keys.

Lint Rule

  1. A lint rule which uses a variable reference parser to display lints based on content of a string

App Specifc Functionality (Sherlock)

  1. A Sherlock "Quick Action" in Code for Declaration creation per message to not have to jump back & forth between the message component and the code.

I would not advise for [3] upfront because why should somebody want go "backwards" the compilation step of the message function. This feels like the wrong DX/UX. Also, thinking about other libraries we need to support further: they also always start on the content/key side. See i18next.

Bildschirmfoto 2024-06-07 um 11.34.42.png


So the mental model is always:

  1. Define declaration in content (message component in our case)
  2. Use declaration in code

Copy link
Member Author

Let's have a short call after lunch :)

Copy link
Contributor

felixhaeberle commented Jun 7, 2024

yep, time? 13:00?

Copy link
Member Author

Summary from the call:

  1. We need to make sure that the communication between dev and ast infromation is working via type lints in m function (maybe show a link to edit or quickfix problems)
    @loris.sigrist how is paraglide doing type checks, lint.
  2. We can work on the extraction of variables

@felixhaeberle
Copy link
Contributor

@NilsJacobsen Can you create follow-up issues that we can close this discussion here?

  • type lints & type checks are probably not Sherlock related anymore and need to be done by either Paraglide or a Lint Rule
  • happy to get a follow-up to required Sherlock functionality aswell

Copy link
Member Author

Gonna talk to Loris about it and then add it to the right team.

Copy link
Collaborator

LorisSigrist commented Jun 17, 2024

I don't think encouraging devs to add extra variables to messages is a good idea as we are not able to consume all possible JS values. What if someone passes a user object instead of just the name? How do we detect the type of a value without reimplementing TS? This makes any "extraction" based approach (like a lint rule) undesirable.

Editing messages & making extra inputs available should be done in the same place that new messages are created. For devs that's Sherlock or files.

Copy link
Member Author

The question is, can we already lint if a user didn't add the required inputs?

Copy link
Collaborator

The extra values would cause a type-error in the code. The dev already sees they're adding extra values & will just updated the message AST with the inputs.

Non-Devs wouldn't benefit from this lint rule since code with type-errors won't be committed.

I don't see a benefit to a lint rule.

Copy link
Member Author

NilsJacobsen commented Jun 17, 2024

The extra values would cause a type-error in the code.

This is already working right?

I don't see a benefit to a lint rule.

I don't think somebody is proposing that 😀

Copy link
Member Author

@felix.haeberle would it be possible to add a link/quickfix to the type error modal via Sherlock? How can these be adjusted?

@felixhaeberle
Copy link
Contributor

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

No branches or pull requests

4 participants