Update lint workflow to exclude ember symbols #38006
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Why: To continue working on the full separation between libCHIP and Ember, we strive to stop using ember symbols. How: By implementing a check for some patterns that match all ember symbols after checking it previously. Then, excluding some specific directories that are needed for now such as `src/app/util`.
Why: The current version excludes every symbol that matches the patterns `emberAf` and `emAf`, this way we provide a way to exclude a specific directory that is currently in use, also to comply with the current requirements How: By updating the `:(exclude)` instances and partitioning the step into two steps.
Why: to improve the experience when running CI/CD jobs, and providing information to the developer of what parts are failing so that they can be fixed more quickly How: by encapsulating the matches in a variable, then checking if it has value or not, and then `echo` the matches. Otherwise, exit the job regularly with code 0.
Why: to avoid redundant searches and fake positives How: by including `.github/` in the exclusions.
Refactor the lint workflow to use a shell script for checking 'ember' and 'emAf' symbols. * Add `scripts/check_symbols.sh` to check for 'ember' and 'emAf' symbols and take exclusion folders as arguments. * Modify `.github/workflows/lint.yml` to remove the jobs "Check for use of 'ember' symbols" and "Check for use of 'emAf' symbols". * Add a job in `.github/workflows/lint.yml` to call the shell script with the same exclusion folders. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/jennygaz/connectedhomeip/tree/update-lint-exclude-ember-symbols?shareId=XXXX-XXXX-XXXX-XXXX).
… improve symbol checking * **scripts/check_symbols.sh** - Modify the script to accept exclusion folders using the `--skip-dir` flag - Update the function to handle the new argument format - Print an error message for unknown options * **.github/workflows/lint.yml** - Remove jobs for checking 'ember' and 'emAf' symbols - Add a job to call the updated shell script with the new exclusion folder format - Remove the extra comment on line 338
Update the excluded directories
add `src/data-model-providers` as an exclusion
Exclude scripts to exclude the script itself
Add execution permission to check_symbols.sh
* Add check symbols script * Provide the places needed to refactor as part of an error message
bzbarsky-apple
requested changes
Mar 14, 2025
| @@ -270,6 +270,25 @@ jobs: | |||
| # to fail (exit nonzero) on match. And we want to exclude this file, | |||
There was a problem hiding this comment.
The comment about git grep makes no sense here, because this step is not using git grep (in this file).
And "this file" is not being excluded; some ancestor directory (not even the closest possible one) is. Please fix the documentation to actually document what's going on.
| done | ||
|
|
||
| ember_matches=$(git grep -I -n '\<ember[A-Za-z0-9_]*' -- './*' "${exclusions[@]}" \ | ||
| ':(exclude).github/*' \ |
There was a problem hiding this comment.
So why are we duplicating exclusions between hardcoded bits here and the command-line arguments?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update the
lint.ymlfile so that it also checks for symbolsemberAfandemAf. This is done by adding a separate script,check_symbols.sh, which searches for uses of those symbols and has the ability to exclude specific directories with the--skip-dirflag.This is the first step of many to solving #36622, but it requires refactoring the places marked by the script to make the script pass and avoid failing future PRs unnecessarily. It is also expected that the job created by this script will fail since we still need the refactor.
Testing
Tested by running the CI/CD updated with the
masterbranch after syncing, in my own fork with Github Actions. Some places could be false positives (e.g., files withindocs) so this is still in the works to fine-tune the exclusions to a place where we can start.refactor_locations.txt