Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat(ast-spec): specifyPunctuatorToken'svalue type#3496

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bradzacher merged 8 commits intotypescript-eslint:masterfromMichaelDeBoey:patch-10
Jun 14, 2021

Conversation

@MichaelDeBoey
Copy link
Contributor

As discussed in#3461 (comment)

This is technically a breaking change, butast-spec is never published, so we shouldn't care I think.

CC/@bradzacher

@typescript-eslint
Copy link
Contributor

Thanks for the PR,@MichaelDeBoey!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day.

@MichaelDeBoey
Copy link
ContributorAuthor

@bradzacher Looking at the error, I don't really know how to fix it tbh.
Can you point me into the right direction please?

@MichaelDeBoeyMichaelDeBoeyforce-pushed thepatch-10 branch 2 times, most recently fromf37ecc1 to71aba31CompareJune 12, 2021 11:57
@MichaelDeBoey
Copy link
ContributorAuthor

@bradzacher@JamesHenry Is there anything I still need to do for this one?

@nx-cloud
Copy link

nx-cloudbot commentedJun 12, 2021•
edited
Loading

Nx Cloud Report

CI ran the following commands for commitedb5087. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

StatusCommand
#000000nx run-many --target=build --all --parallel
#000000nx run-many --target=typecheck --all --parallel

Sent with 💌 fromNxCloud.

@nx-cloud
Copy link

Nx Cloud Report

CI ran the following commands for commit71aba31. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

StatusCommand
#000000nx run-many --target=build --all --parallel
#000000nx run-many --target=typecheck --all --parallel

Sent with 💌 fromNxCloud.

Copy link
Member

@bradzacherbradzacher left a comment

Choose a reason for hiding this comment

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

Ideally I wouldn't want us to have to keep track of this 60 element union ourselves AND duplicate it into a separate interface mapPunctuatorTokenToText or other.

So if you can refactor the types so that we have

  • PunctuatorTokenToText,
  • TokenToText extends PunctuatorTokenToText, and
  • PunctuatorToken['value'] === values<PunctuatorTokenToText>

I'm all for that approach

@bradzacherbradzacher added the enhancementNew feature or request labelJun 13, 2021
@bradzacherbradzacher changed the titlefix(ast-spec): specifyPunctuatorToken'svalue typefeat(ast-spec): specifyPunctuatorToken'svalue typeJun 13, 2021
@MichaelDeBoey
Copy link
ContributorAuthor

@bradzacher Where do you want me to havePunctuatorTokenToText to live then?
TokenToText is living intypescript-estree andast-spec currently doesn't have any dependencies

@bradzacher
Copy link
Member

ast-spec is not a published package. It is built and copied across to thetypes package.
So we don't care if we add dependencies to it.

As long as you explicitly mark the TS import as animport type then it should be built and copied fine.

You can add this block to thetypes package.json to ensure we have an implicit peer dependency on it.

"peerDependenciesMeta": {
"typescript": {
"optional":true
}
},

MichaelDeBoey reacted with thumbs up emoji

@MichaelDeBoey
Copy link
ContributorAuthor

@bradzacher Just pushed the commits you asked for.

I'm happy to change some things around if needed.

Copy link
Member

@bradzacherbradzacher left a comment

Choose a reason for hiding this comment

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

lgtm - thanks for this!

MichaelDeBoey reacted with thumbs up emoji
@bradzacherbradzacher merged commitfdb1d81 intotypescript-eslint:masterJun 14, 2021
@MichaelDeBoeyMichaelDeBoey deleted the patch-10 branchJune 14, 2021 20:58
@MichaelDeBoey
Copy link
ContributorAuthor

Always happy to help out!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@bradzacherbradzacherbradzacher approved these changes

Assignees

No one assigned

Labels

enhancementNew feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichaelDeBoey@bradzacher
[8]ページ先頭
©2009-2025 Movatter.jp