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

Add option to warn when missing semicolons #1402

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

NduatiK
Copy link
Contributor

@NduatiK NduatiK commented Aug 4, 2024

Addresses #182

When calling the parser you can now specify that warnings be emitted on the console if semicolons are missing. When parsing using the expect_semicolons? option:

RulesParser.parse(input, expect_semicolons?: true, ...opts)

The value would be false (or not set) in most places, but set to true when parsing the style attribute on elements.

@NduatiK NduatiK marked this pull request as draft August 4, 2024 20:00
@bcardarella
Copy link
Collaborator

what would be the use case for false in the expect_semicolons ?

@NduatiK
Copy link
Contributor Author

NduatiK commented Aug 4, 2024

From my understanding we only wanted the warning on the style attribute but not on the ~SHEET sigil. Is this incorrect?

@bcardarella
Copy link
Collaborator

As we want to keep our implementation similar to CSS:

Semicolons are not strictly required at the end of each line in a CSS file. However, they are necessary to separate declarations within a CSS rule. For example:

h1 {
  color: blue;
  font-size: 20px;
}

In this example, the semicolons are needed to separate the color and font-size declarations. If there is only one declaration, the semicolon is optional:

h1 {
  color: blue
}

Despite this, it's considered good practice to always include semicolons, even for the last declaration in a rule, to avoid potential errors and maintain consistency in the code.

Also shouldn't this be in live_view_native_stylesheet instead?

@carson-katri
Copy link
Contributor

Isn’t the code inside a rule supposed to match the target platform? In Swift you don’t need a semicolon at the end of each statement, as long as they are separated by a newline.

@bcardarella
Copy link
Collaborator

bcardarella commented Aug 6, 2024

I'm not super tied to the implementation and I think there is time for us to debate on this.

If we were to lean in the direction of the upstream UI framework then we should go all-in and rather than newline use . and that could also extend to the style attr too.

On the other side, separating the top line modifiers/rules by ; gives us a single API syntax for stylesheets that is a common-lingua.

@NduatiK
Copy link
Contributor Author

NduatiK commented Aug 6, 2024

Also shouldn't this be in live_view_native_stylesheet instead?

The semicolons are delimiters between modifiers, the stylesheet lib doesn't know about modifiers and so this functionality has to be in the client libraries. Once Jetpack arrives and we consolidate functionality into live_view_native_stylesheet, it will live there.


I'm not super tied to the implementation and I think there is time for us to debate on this.

I'll leave it as configurable for now.

@NduatiK NduatiK marked this pull request as ready for review August 8, 2024 12:47
@NduatiK NduatiK force-pushed the nk-warn-when-missing-semicolons branch from f2caced to 6129156 Compare August 9, 2024 11:45
@bcardarella
Copy link
Collaborator

@NduatiK thank you, let me dig in after v0.3 is released. I want us to come to a consensus if we will go the route of leaning into the native UI framework's own deliminator or semicolons.

@bcardarella
Copy link
Collaborator

I need to look into what Jetpack and possibly WinUI3 (Maui?) need for this as I want to make sure this doesn't paint us in a corner with a single framework

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

Successfully merging this pull request may close these issues.

3 participants