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

Replace == in the views with === #1636

Merged
merged 20 commits into from
Jan 21, 2025
Merged

Replace == in the views with === #1636

merged 20 commits into from
Jan 21, 2025

Conversation

Jozzey
Copy link
Contributor

@Jozzey Jozzey commented Jan 20, 2025

It has been spotted in a few views that we have been using == rather than ===.

Generally, we use the strict comparison === to compare content & type unless it is necessary to perform a loose equality comparison.

It looks like it is not necessary to use == in the views. So where applicable they will be updated to ===.

It has been spotted in a few views that we have been using `==` rather than `===`.

Generally we use the strict comparison `===` to compare content & type  unless it is absolutely necessary to perform a loose equality comparison.

It looks like it is not necessary to use `==` in the views. So where applicable they will be updated to `===`.
@Jozzey Jozzey added the housekeeping Refactoring, tidying up or other work which supports the project label Jan 20, 2025
@Jozzey Jozzey self-assigned this Jan 20, 2025
@Jozzey Jozzey marked this pull request as ready for review January 20, 2025 14:42
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

I'm happy to approve. But only if the acceptance tests report zero failures when run.

Could you add a screenshot of the test summary to the PR to confirm this?

Jozzey and others added 2 commits January 21, 2025 12:57
Running the acceptance tests highlighted an issue with the view `app/views/bill-runs/review/review-licence.njk`. It was breaking when I updated line 237 to this `{% if matchedReturns.length === 0 and unmatchedReturns.length === 0 %}`.

After some investigation I found that `matchedReturns` & `unmatchedReturns` are arrays, not numbers. The `==` seemed to work when assessing if the array was empty. Just goes to show how loose the `==` matching is.

 I've therefore updated it to what I expect it should have been in the first place unmatchedReturns.length === 0 and it now works fine.
@Jozzey
Copy link
Contributor Author

Jozzey commented Jan 21, 2025

The acceptance tests picked up an issue with app/views/bill-runs/review/review-licence.njk which has been fixed. There is just 1 failing acceptance test, but the same test also fails when run on the main branch so is not related to the changes in this PR.

Screenshot 2025-01-21 at 14 17 59
Screenshot 2025-01-21 at 14 18 08

@Jozzey Jozzey requested a review from Cruikshanks January 21, 2025 14:23
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

@Jozzey Jozzey merged commit 63adcfe into main Jan 21, 2025
7 checks passed
@Jozzey Jozzey deleted the update-double-equals branch January 21, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Refactoring, tidying up or other work which supports the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants