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

fix: update strings.json #2681

Closed
wants to merge 12 commits into from
Closed

Conversation

danielbrunt57
Copy link
Collaborator

@danielbrunt57 danielbrunt57 commented Nov 16, 2024

  • Removed: services: clear_history
  • Changed: miscellaneous wording changes

@alandtse
Copy link
Owner

This seems to be tied to another PR if you're introducing a warning deprecated_yaml_configuration.

@danielbrunt57
Copy link
Collaborator Author

danielbrunt57 commented Nov 18, 2024

Yes, the deprecated_yaml_configuration key is required by PR #2685

@alandtse
Copy link
Owner

So changes should be atomic. So any string changes for that pr should be in the PR. You shouldn't require two PRs to get the effect.

@danielbrunt57
Copy link
Collaborator Author

Alright, I'll amend both PR's then...

@danielbrunt57
Copy link
Collaborator Author

My thinking was that both PR's would be merged at the same time thus no harm. Or the strings PR merged first then the deprecated PR following that, but I'll conform...

@alandtse
Copy link
Owner

Also, after the other one goes through, please explain why you're updating the strings better. Update doesn't explain anything. If we needed to review this 3 months later, it doesn't explain what has changed any more than the fact a file has been changed. It forces someone to actually read the changes to guess what was intended.

@danielbrunt57
Copy link
Collaborator Author

danielbrunt57 commented Nov 20, 2024

If you prefer, I can close my PR for deprecate and let you merge strings. Then I update my deprecate branch to your new dev and submit a new PR with new strings addition and config changes for deprecation...

@danielbrunt57
Copy link
Collaborator Author

danielbrunt57 commented Nov 24, 2024

I'm going to close this one as it is basically just cosmetic + remove a deprecated section.
I think it's more important to move ahead on the yaml deprecation PR plus get the merged 4.13.8 reversion out there.

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