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.

Sign up for GitHub

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

Jump to bottom

Fix broken exception messages #64

Merged
merged 1 commit into from Dec 23, 2023
Merged

Fix broken exception messages #64

merged 1 commit into from Dec 23, 2023

Conversation

lrandall-godaddy
Copy link
Contributor

@lrandall-godaddy lrandall-godaddy commented Dec 6, 2023 •
edited

Changes introduced by 09edfd4 introduced some issues with exception messages:

  • exceptions raised by #set do not include the address (as it interpolates an instance variable that has not yet been set)
  • some messages include the address even though the exception is unrelated to the address (for example a prefix error)

@lrandall-godaddy lrandall-godaddy changed the title Fix invalid error messages introduced by 09edfd4 Fix invalid exception messages Dec 6, 2023
@lrandall-godaddy lrandall-godaddy changed the title Fix invalid exception messages Fix broken exception messages Dec 6, 2023
@hsbt
Copy link
Member

hsbt commented Dec 7, 2023

What's broken? Please fill your issue in description at least.

@lrandall-godaddy
Fix regressions in exception messages
74a0431 
Changes introduced by 09edfd4 have broken some exception
messages, and added the address as an unnecessary
suffix in others.
@lrandall-godaddy
Copy link
Contributor Author

@hsbt have added an explanation of what this corrects

@hsbt
Copy link
Member

hsbt commented Dec 13, 2023

@jeremyevans Can you look this?

jeremyevans
jeremyevans approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@jeremyevans jeremyevans 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 OK with this. Some of the cases where @addr is removed, it may be useful to substitute with mask or prefix instead.

@knu knu merged commit 24557e9 into ruby:master Dec 23, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyevans jeremyevans jeremyevans approved these changes

@knu knu knu approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lrandall-godaddy @hsbt @jeremyevans @knu