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

Remove unused data keys #1227

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Remove unused data keys #1227

merged 4 commits into from
Mar 28, 2024

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Mar 27, 2024

🛠 Summary of changes

Removes unused values from settings.yml files within _data, and adds a test case to ensure that keys are consistent between all languages.

Related comment: #1226 (comment)

Identifying lack of use required some manual effort, specifically searching settings.[key]

📜 Testing Plan

bundle exec rspec spec/data_spec.rb

Verify no lingering references in code to removed keys.

@aduth aduth requested a review from a team March 27, 2024 21:54
end
end

def hash_keys(hash, parent_keys: [], all_keys: [])
Copy link
Contributor Author

@aduth aduth Mar 27, 2024

Choose a reason for hiding this comment

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

This implementation built upon a similar one in the IdP:

https://github.com/18F/identity-idp/blob/3e4246d/spec/i18n_spec.rb#L245-L256

(We don't care about the values here, only the keys)

zachmargolis
zachmargolis previously approved these changes Mar 27, 2024
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

spec/data_spec.rb Outdated Show resolved Hide resolved
Co-authored-by: Zach Margolis <[email protected]>
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +8 to +9
first_file_keys, *other_file_keys = keys
first_file, first_keys = first_file_keys
Copy link
Contributor

@zachmargolis zachmargolis Mar 28, 2024

Choose a reason for hiding this comment

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

At the risk of adding yet another dismissed review, one way to save a line and remove an intermediate variable

Suggested change
first_file_keys, *other_file_keys = keys
first_file, first_keys = first_file_keys
(first_file, first_keys), *other_file_keys = keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, yeah, the review dismissal really discourages feedback / iteration 😕

Fortunately, I think I prefer it as-is over the alternative. The multi-level destructuring feels a little hard to read, and this is a little more consistent with the _file_keys pairing between "first" and "other", and the subsequent destructure of block parameters on line 11.

@aduth aduth merged commit a5495f7 into main Mar 28, 2024
14 checks passed
@aduth aduth deleted the aduth-rm-unused-data-keys branch March 28, 2024 17:15
jc-gsa pushed a commit that referenced this pull request Apr 2, 2024
* Remove unused data keys

* Remove debugger pry require

* Compare all to first with clear aggregated error

* Attribution

Co-authored-by: Zach Margolis <[email protected]>

---------

Co-authored-by: Zach Margolis <[email protected]>
mitchellhenke pushed a commit that referenced this pull request May 6, 2024
* Remove unused data keys

* Remove debugger pry require

* Compare all to first with clear aggregated error

* Attribution

Co-authored-by: Zach Margolis <[email protected]>

---------

Co-authored-by: Zach Margolis <[email protected]>
mitchellhenke pushed a commit that referenced this pull request May 6, 2024
* Remove unused data keys

* Remove debugger pry require

* Compare all to first with clear aggregated error

* Attribution

Co-authored-by: Zach Margolis <[email protected]>

---------

Co-authored-by: Zach Margolis <[email protected]>
mitchellhenke pushed a commit that referenced this pull request May 6, 2024
* Remove unused data keys

* Remove debugger pry require

* Compare all to first with clear aggregated error

* Attribution

Co-authored-by: Zach Margolis <[email protected]>

---------

Co-authored-by: Zach Margolis <[email protected]>
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.

3 participants