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

feat: Ability to skip "meta" tokens (whitespace, comments, attributes) #7559

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Dec 15, 2023

I got this idea while reviewing #7395, I believe it could be helpful to achieve this (jump directly from #[ to meaningful element described with that attribute, then run attribute analysis).

@Wirone Wirone self-assigned this Dec 15, 2023
@coveralls
Copy link

Coverage Status

coverage: 94.908% (-0.009%) from 94.917%
when pulling fea7d88 on Wirone:codito/navigate-to-non-meta-tokens
into 7de792f on PHP-CS-Fixer:master.

@Wirone Wirone requested review from keradus and removed request for kubawerlos January 6, 2024 12:04
@@ -572,6 +584,43 @@ public function getNonWhitespaceSibling(int $index, int $direction, ?string $whi
}
}

/**
Copy link
Member

@keradus keradus Jan 7, 2024

Choose a reason for hiding this comment

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

In general, for any code:

I struggle to understand the need for code without that code being used. For now, it looks only as a dead code
( and I would love to have automation to prevent any deadcode being in repo, as side note ;) )

I also fail to understand how common this will get used - is it's needed only for very limited usecase (and thus encapsulated into method of class, where will be the usage) or spread across widely (thus should be exposed via "main" Tokens).

For that, I would love to see actual usage of code, eg in the PR that is actually using it.
We could also have a PR adding Tokens code (without usage), and other feature PR build on top, that shows how it will be used.
Or other way around, merge feature PR without having this logic externalised to Tokens, and then refactoring PR with code moving to Tokens and getting use in multiple rules.

@@ -572,6 +584,43 @@ public function getNonWhitespaceSibling(int $index, int $direction, ?string $whi
}
}

/**
* Get index for closest sibling token which is non meta (whitespace, comment, PHP8 attribute).
Copy link
Member

Choose a reason for hiding this comment

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

curious - is meta some formal naming here, or only a word you picked? (happy with both, just curious)

/**
* Get index for closest sibling token which is non meta (whitespace, comment, PHP8 attribute).
*
* @param int $index token index
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param int $index token index

Copy link

github-actions bot commented Apr 7, 2024

Since this pull request has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 14 days.

Please keep your branch up-to-date by rebasing it when main branch is ahead of it, thanks in advance!

@github-actions github-actions bot added status/to verify issue needs to be confirmed or analysed to continue and removed status/stale labels Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/to verify issue needs to be confirmed or analysed to continue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants