Skip to content

Chore: Mark SourceCode getComments() method as deprecated (fixes #13293)#13296

Merged
kaicataldo merged 1 commit intoeslint:masterfrom
SuperOleg39:issue13293
May 19, 2020
Merged

Chore: Mark SourceCode getComments() method as deprecated (fixes #13293)#13296
kaicataldo merged 1 commit intoeslint:masterfrom
SuperOleg39:issue13293

Conversation

@SuperOleg39
Copy link
Copy Markdown
Contributor

@SuperOleg39 SuperOleg39 commented May 13, 2020 •
edited by mdjermanovic
Loading

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Update source-code.js getComments method:

Program node exclude leading and trailing comments from our range, and i add special conditions, when given node has parent Program, leading and trailing comments tokens always belong to this node

@jsf-clabot
Copy link
Copy Markdown

jsf-clabot commented May 13, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated Bot added the triage An ESLint team member will look at this issue soon label May 13, 2020
@SuperOleg39 SuperOleg39 changed the title Fix: Change SourceCode usage deprecated node properties start and end… Fix: Change SourceCode usage deprecated node properties start and end to range (fixes #13293) May 13, 2020
@SuperOleg39 SuperOleg39 changed the title Fix: Change SourceCode usage deprecated node properties start and end to range (fixes #13293) Fix: Change SourceCode usage deprecated node properties (fixes #13293) May 13, 2020
kaicataldo
kaicataldo suggested changes May 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Given that SouceCode#getComments() is deprecated, we won't be accepting any more PRs to change the behavior. A PR updating the JSDoc block to include an @deprecated tag would be welcome, though!

@kaicataldo kaicataldo added works as intended The behavior described in this issue is working correctly and removed triage An ESLint team member will look at this issue soon labels May 13, 2020
@kaicataldo kaicataldo mentioned this pull request May 13, 2020
Closed
@SuperOleg39 SuperOleg39 changed the title Fix: Change SourceCode usage deprecated node properties (fixes #13293) Docs: Mark SourceCode getComments() method as deprecated (fixes #13293) May 14, 2020
@SuperOleg39
Copy link
Copy Markdown
Contributor Author

SuperOleg39 commented May 14, 2020

Given that SouceCode#getComments() is deprecated, we won't be accepting any more PRs to change the behavior. A PR updating the JSDoc block to include an @deprecated tag would be welcome, though!

Thank you, update PR!

@SuperOleg39 SuperOleg39 requested a review from kaicataldo May 14, 2020 07:41
kaicataldo
kaicataldo reviewed May 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Can we change the commit message from Docs: to Chore:? We reserve Docs: for publicly visible documentation.

Otherwise, this LGTM!

@SuperOleg39 SuperOleg39 changed the title Docs: Mark SourceCode getComments() method as deprecated (fixes #13293) Chore: Mark SourceCode getComments() method as deprecated (fixes #13293) May 18, 2020
@SuperOleg39
Copy link
Copy Markdown
Contributor Author

SuperOleg39 commented May 18, 2020

Done!

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing and removed works as intended The behavior described in this issue is working correctly labels May 18, 2020
mdjermanovic
mdjermanovic approved these changes May 18, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mdjermanovic mdjermanovic 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!

@SuperOleg39
Copy link
Copy Markdown
Contributor Author

SuperOleg39 commented May 18, 2020

☺️

@kaicataldo
Copy link
Copy Markdown
Member

kaicataldo commented May 19, 2020

LGTM, thank you!

@kaicataldo kaicataldo merged commit 72a4e10 into eslint:master May 19, 2020
@SuperOleg39 SuperOleg39 deleted the issue13293 branch May 20, 2020 08:00
@eslint-deprecated eslint-deprecated Bot locked and limited conversation to collaborators Nov 16, 2020
@eslint-deprecated eslint-deprecated Bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mdjermanovic mdjermanovic mdjermanovic approved these changes

+1 more reviewer

@kaicataldo kaicataldo kaicataldo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SuperOleg39 @jsf-clabot @kaicataldo @mdjermanovic