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

lint(validators): refactor len conditions #215

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Mar 11, 2021

In our validation procs, there are conditions like

if data.hasKey(key):
  if data[key].kind == JString:

where the condition being true represents the "happy path".

To improve readability, let's try to do this consistently for every
condition apart from the innermost one; it's easier to see the "happy
path" when it minimises the number of else.

This commit refactors the len conditions accordingly, so that we
prefer len > 0 and use len == 0 only when there is no else.

This commit also changes a var to a let to better convey that we
don't mutate.

@ee7 ee7 changed the title Lint validators refactor len conditions lint(validators): refactor len conditions Mar 11, 2021
In our validation procs, there are conditions like
```Nim
if data.hasKey(key):
  if data[key].kind == JString:
```
where the condition being `true` represents the "happy path".

To improve readability, let's try to do this consistently for every
condition apart from the innermost one; it's easier to see the "happy
path" when it minimises the number of `else`.

This commit refactors the `len` conditions accordingly, so that we
prefer `len > 0` and use `len == 0` only when there is no `else`.

This commit also changes a `var` to a `let` to better convey that we
don't mutate.
@ee7 ee7 force-pushed the lint-validators-refactor-len-conditions branch from 23a295e to 910feca Compare March 11, 2021 13:58
@ee7 ee7 marked this pull request as ready for review March 11, 2021 14:00
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

LGTM

@ee7
Copy link
Member Author

ee7 commented Mar 11, 2021

I've again verified that this commit keeps the output of configlet lint the same on every track. I intend to add that to CI eventually, although it can't be a "test" exactly - some commits will be expected to change the output of configlet lint.

@ee7 ee7 merged commit 6c0c041 into exercism:main Mar 11, 2021
@ee7 ee7 deleted the lint-validators-refactor-len-conditions branch March 11, 2021 15:09
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