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

Extend quoting functionality to include polyvariant keywords #877

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

WhyThat
Copy link
Contributor

@WhyThat WhyThat commented Dec 19, 2023

Hey,
Following up #870, we have now extended the functionality to also wrap polyvariant keywords with quotes.

@zth
Copy link
Collaborator

zth commented Dec 19, 2023

Great stuff! Can you add a changelog?

@WhyThat
Copy link
Contributor Author

WhyThat commented Dec 19, 2023

@zth Done :)
Just thinking about it, we could use fileNameHasUnallowedChars to detect if we have to wrap exotic polyvariant, wyt ?

@zth
Copy link
Collaborator

zth commented Dec 19, 2023

@zth Done :) Just thinking about it, we could use fileNameHasUnallowedChars to detect if we have to wrap exotic polyvariant, wyt ?

I don't think that's equivalent to what we do, right? Doesn't that check that the string is capitalized etc?

@zth zth merged commit d77ca15 into rescript-lang:master Dec 19, 2023
5 checks passed
@WhyThat
Copy link
Contributor Author

WhyThat commented Dec 19, 2023

I don't think that's equivalent to what we do, right? Doesn't that check that the string is capitalized etc?

Seems like it match the same things after testing these cases :

let printMaybeExoticIdent txt =
  (* fileNameHasUnallowedChars "not good" (* => true *); *)
  (* fileNameHasUnallowedChars "not-good" (* => true *); *)
  (* fileNameHasUnallowedChars "good"(* => false *); *)
  (* fileNameHasUnallowedChars "Good"(* => false *); *)
  (* fileNameHasUnallowedChars "123"(* => false *); *)
  match (Res_token.isKeywordTxt txt, fileNameHasUnallowedChars txt) with
  | true, _ | _, true -> "\"" ^ txt ^ "\""
  | _ -> txt

test are unchanged with this, but nevermind

@WhyThat WhyThat deleted the polyvar-exhaustive-switch branch December 19, 2023 14:45
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.

2 participants