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

refactor: add parse modes type hints, minor refactoring #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

evermake
Copy link

@evermake evermake commented Feb 4, 2023

Changes made:

  • wellKnownParseModesMap map was replaced with knownParseModes object;
  • buildTransformer (a.k.a. parseMode) arrow function was transformed to regular function and also was overloaded to show type hints of available parse modes;
  • src/transformer.ts file formatted with Deno.

src/transformer.ts Outdated Show resolved Hide resolved
@evermake
Copy link
Author

evermake commented Feb 4, 2023

@KnorpelSenf , I've simplified code. Could someone review?

Also, is it okay, that deno check found 5 type errors here?

src/transformer.ts Outdated Show resolved Hide resolved
@KnorpelSenf
Copy link
Member

Also, is it okay, that deno check found 5 type errors here?

No. Please fix them :)

@KnorpelSenf
Copy link
Member

@KnightNiwrem this may be the day to set up CI for your repository :D

@evermake
Copy link
Author

evermake commented Feb 4, 2023

Also, is it okay, that deno check found 5 type errors here?

No. Please fix them :)

I guess they were before me... I haven't figured out their reasons yet.

Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after type errors and potential linting errors are fixes.

The only question I have: are we even allowed to change the type from string to ParseMode? I think this is a breaking change because it will prevent existing code from compiling. That would mean that this PQ would require a new major version. I am not sure if @KnightNiwrem is fine with that.

If not, we can consider adding a helper type that grammY uses.

type StringWithSuggestions<S extends string> = (string & {}) | S; // permits `string` but gives hints

@KnorpelSenf
Copy link
Member

@evermake are you still interested in fixing this up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: In progress
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants