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

New terminal_close_linter #2276

Merged
merged 5 commits into from
Nov 14, 2023
Merged

New terminal_close_linter #2276

merged 5 commits into from
Nov 14, 2023

Conversation

MichaelChirico
Copy link
Collaborator

Part of #884

We might consider renaming this to just close_linter(), to flexibility in the future for more lints associated with use of close(). E.g., we might make this stricter by default and require all close() calls to be inside on.exit() hooks.

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a1e2f1f) 99.63% compared to head (435299f) 99.63%.

❗ Current head 435299f differs from pull request most recent head bf2efc1. Consider uploading reports for the commit bf2efc1 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2276   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files         114      114           
  Lines        5258     5258           
=======================================
  Hits         5239     5239           
  Misses         19       19           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AshesITR
Copy link
Collaborator

Is this vectorized to detect multiple (trailing) close() calls? Should it be?

@MichaelChirico
Copy link
Collaborator Author

Is this vectorized to detect multiple (trailing) close() calls? Should it be?

Not now... can't say I've encountered that either. I would reserve for a follow-up if needed.

@AshesITR
Copy link
Collaborator

All right. Can we add a metadata test here as well? Maybe with an example containing two close statements to show which one lints?

@MichaelChirico
Copy link
Collaborator Author

All right. Can we add a metadata test here as well? Maybe with an example containing two close statements to show which one lints?

SG. Any opinion on terminal_close_linter() vs. close_linter()?

@AshesITR
Copy link
Collaborator

Hmm not sure where this might go. I can imagine a connection_linter() requiring on.exit(close(...)) immediately after any connection-opening code unless the connection is returned by the function.

My gut feeling says let's keep the name for now and see to deprecating / renaming if and when the need arises.

@MichaelChirico MichaelChirico merged commit 984a399 into main Nov 14, 2023
20 checks passed
@MichaelChirico MichaelChirico deleted the terminal_close branch November 14, 2023 05:46
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