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

tidy: invert negated calls to Option.isEmpty()/isDefined() #1289

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alxndrsn
Copy link
Contributor

No description provided.

@alxndrsn
Copy link
Contributor Author

@matthew-white @ktuite here's an example of a code pattern that a custom "lint" rule might be useful for, e.g.

$ git grep -En '!.*is(Defined|Empty)'
lib/http/preprocessors.js:34:      if (!session.isDefined()) return onFailure();
lib/http/preprocessors.js:123:      if ((cxt == null) || !cxt.auth.session.isDefined()) return;
lib/model/migrations/20191007-01-backfill-client-audits.js:30:      if (!auditNode.isDefined()) return;
lib/model/query/entities.js:281:  if (!serverEntity.isDefined()) {
lib/model/query/entities.js:366:    if (!maybeDef.isDefined()) {
lib/model/query/entities.js:391:    if (!baseEntityVersion.isDefined()) {
lib/model/query/entities.js:427:  if (!form.isDefined())
lib/model/query/entities.js:445:  if (!submission.isDefined())
lib/resources/oidc.js:193:  if (!userOption.isDefined()) return;
lib/task/analytics.js:28:        .then((au) => ((!isEmpty(au) && au.value.details.success === true && !force)

@alxndrsn
Copy link
Contributor Author

This kind of pattern could also be detected with a tool like semgrep.

@matthew-white
Copy link
Member

here's an example of a code pattern that a custom "lint" rule might be useful for

This kind of pattern could also be detected with a tool like semgrep.

I agree that removing the negation makes the code more readable. That said, there were only 10 instances of it, so I don't think it's worth the trouble of crafting something custom to detect it. We can keep an eye out for it during code review.

@alxndrsn alxndrsn marked this pull request as ready for review November 11, 2024 16:42
@alxndrsn
Copy link
Contributor Author

I don't think it's worth the trouble of crafting something custom to detect it

It would be good to reduce friction of this kind of check to near-zero so that there are less unwritten rules & hidden gotchas. I'll continue to collect more examples of these.

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

Successfully merging this pull request may close these issues.

2 participants