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 code #2000

Merged
merged 10 commits into from
Dec 4, 2024
Merged

Tidy code #2000

merged 10 commits into from
Dec 4, 2024

Conversation

threedaymonk
Copy link
Collaborator

  • Disabled specs are enabled, fixed, or removed as appropriate
  • Commented-out code is deleted
  • Translations are normalised using i18n-tasks

Copy link

Test coverage

Line: 87.1%
Branch: 77.21%

@threedaymonk
Copy link
Collaborator Author

SonarCloud has some spurious complaints about new code that doesn't exist!

@threedaymonk threedaymonk force-pushed the tidy-unused-specs branch 2 times, most recently from 089bf38 to 2bd5e15 Compare November 26, 2024 17:38
Copy link

Test coverage

Line: 87.27%
Branch: 77.33%

Copy link
Contributor

@serenaabbott11 serenaabbott11 left a comment

Choose a reason for hiding this comment

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

Happy to approve once we've had a quick look to make sure nothing has caused issues for the yaml file (can either be you confirming on your local machine, or putting it on to dev and I can have a play around)

Copy link
Contributor

@serenaabbott11 serenaabbott11 left a comment

Choose a reason for hiding this comment

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

Had a play around on dev and all looks fine

These specs were disabled when phone number validation was removed in
commit 3d36c96. They will always be in
the version history, so we don't need them preserved here.
This contained only one spec, which was disabled when the validation was
removed in commit 6f70ffd.
There never was a spec here. The intention is lost in time, like
tears in rain.
This was disabled in commit 7d576e8
with the message "Test by disabling the suspected culprit".
Test what, though? And was this the culprit? We can't know.
This spec failed because there was a confusion between keyword arguments
and old-style hash arguments. It is fixed by using keyword arguments
consistently.
We have version history. We don't need to keep unused code around in
comments "just in case".
Done by running:
i18n-tasks normalize
This is extremely slow, and wasn't being run. We still have the same
functionality with i18n-tasks.
The classes referred to by these specs were deleted in the Great Email
Refactor of October 2023, commit 988bc05.
These were disabled a year ago in commit d57130d, which adds other
tests for the behaviour of attachments. These specs are no longer needed
nor useful.
Copy link

sonarqubecloud bot commented Dec 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copy link

github-actions bot commented Dec 3, 2024

Test coverage

Line: 87.03%
Branch: 77.23%

@threedaymonk threedaymonk merged commit daf1c70 into main Dec 4, 2024
24 of 25 checks passed
@threedaymonk threedaymonk deleted the tidy-unused-specs branch December 4, 2024 11:11
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