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

Firestore: Remove obsolete NOLINT comments #14347

Merged
merged 4 commits into from
Jan 20, 2025

Conversation

dconeybe
Copy link
Contributor

This PR removes NOLINT comments from the code base that are obsolete as of #14320, which deleted obsolete c++11 and c++14 lint checks.

#no-changelog

@dconeybe dconeybe self-assigned this Jan 14, 2025
@dconeybe dconeybe force-pushed the dconeybe/CppLintRemoveObsoleteNoLintComments branch from 96fd28a to 605d689 Compare January 15, 2025 03:58
@dconeybe dconeybe marked this pull request as ready for review January 17, 2025 15:34
scripts/cpplint.py Show resolved Hide resolved
@dconeybe dconeybe requested a review from cherylEnkidu January 17, 2025 17:44
@dconeybe dconeybe merged commit deb3fd0 into main Jan 20, 2025
36 checks passed
@dconeybe dconeybe deleted the dconeybe/CppLintRemoveObsoleteNoLintComments branch January 20, 2025 17:15
@ncooke3
Copy link
Member

ncooke3 commented Jan 22, 2025

I think this broken the checkCI on main:

Firestore/core/test/unit/util/executor_test.cc:21:  Found C system header after C++ system header. Should be: executor_test.h, c system, c++ system, other.  [build/include_order] [4]
Firestore/core/test/unit/util/schedule_test.cc:20:  Found C system header after C++ system header. Should be: schedule_test.h, c system, c++ system, other.  [build/include_order] [4]

@ncooke3
Copy link
Member

ncooke3 commented Jan 22, 2025

Hi @dconeybe, does the above suggestion make sense to you?

@dconeybe
Copy link
Contributor Author

Hi @dconeybe, does the above suggestion make sense to you?

Possibly, yes. It looks like the "lint" check tries to be smart and only scan files that have "changed". I must have changed those files in this PR but not run the lint check. I'll open a PR to fix this. It's trivial.

@dconeybe
Copy link
Contributor Author

Opened #14374 to fix the lint errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants