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

Several Improvements #10

Merged
merged 11 commits into from
May 21, 2022
Merged

Several Improvements #10

merged 11 commits into from
May 21, 2022

Conversation

graphemecluster
Copy link
Contributor

@graphemecluster graphemecluster commented May 11, 2022

It may be split into a couple of small PRs if you prefer.

By the way, are there any reasons to remove the documentations in lib.es2015.reflect.d.ts?

@uhyo
Copy link
Owner

uhyo commented May 12, 2022

Thanks for your interest in contributing 🙂

By the way, are there any reasons to remove the documentations in lib.es2015.reflect.d.ts?

No, we don't intend to reduce documentations from the original typings. If there's any, it is unintentional mistake. 😨

@graphemecluster graphemecluster marked this pull request as ready for review May 13, 2022 01:13
@graphemecluster graphemecluster changed the title [WIP] Several Improvements Several Improvements May 13, 2022
@graphemecluster
Copy link
Contributor Author

I just make it ready for review, but I still find it not perfect enough.

If this PR is separated to multiple PRs by myself, it will probably be too many, so please review and choose what you would like to separate.

And please help me think of a good PR title.

Also, I would like to confirm whether I should not commit the generated files.

@graphemecluster
Copy link
Contributor Author

I separated yesterday's commit into several small commits for your convenience of reviewing.

And I found two things to be improved:

  • Remove the top line with triple-slash directive /// <reference no-default-lib="true"/> in the diff docs.
  • Build the library base on each method declaration, so that we don't need to copy the abundant methods that we don't want to change and prevent accidentally forgetting to declare some methods

@graphemecluster
Copy link
Contributor Author

It seems that npm test in the workflow is still using the old type definitions such that the check had failed. 🈚️

@uhyo
Copy link
Owner

uhyo commented May 13, 2022

Thanks, currently you need to commit generated type defs (generated by build:lib script).
Sorry for lack of documents 😢

@graphemecluster
Copy link
Contributor Author

By the way, I wonder why you only include the build:package script in the workflow. 🤔

@graphemecluster
Copy link
Contributor Author

I suddenly come out with an idea that we can add a noOfCaptureGroups type parameter to the replace and replaceAll methods since the user should know how many capture groups they have. It is impractical to have a parameter typed string | number. The other parameters at the end are seldom used either.
And we don’t need to wait until microsoft/TypeScript#45972 being solved too.

@uhyo
Copy link
Owner

uhyo commented May 21, 2022

Sorry for late response, overall LGTM!

By the way, I wonder why you only include the build:package script in the workflow. 🤔

Maybe because changing committed source code during the test isn't quite good.

Build the library base on each method declaration, so that we don't need to copy the abundant methods that we don't want to change and prevent accidentally forgetting to declare some methods

Yes 😢 I want to implement this when I have time.

I suddenly come out with an idea that we can add a noOfCaptureGroups type parameter

That's interesting, but one worrying point is that user could lie about this... but it'd be fine if the current implementation already allows lies.

@uhyo uhyo merged commit 5ee1b5f into uhyo:master May 21, 2022
@graphemecluster
Copy link
Contributor Author

Yes 😢 I want to implement this when I have time.

Looking forward to your implementation! 😆

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