Skip to content

Fix exception message for keyNames parameter to WikiPage::getAllSection#133

Merged
waldyrious merged 1 commit intohamstar:masterfrom
Xymph:133-fix-exception-msg
Aug 29, 2021
Merged

Fix exception message for keyNames parameter to WikiPage::getAllSection#133
waldyrious merged 1 commit intohamstar:masterfrom
Xymph:133-fix-exception-msg

Conversation

@Xymph
Copy link
Copy Markdown
Collaborator

@Xymph Xymph commented Aug 27, 2021

While reviewing the exception/error handling situation this one jumped out just now. The single quotes meant that $keyNames was shown verbatim, not its value. Now the message clarifies which parameter has what unexpected value.

The exception type is also changed to the more specific and appropriate UnexpectedValueException.

@Xymph Xymph requested a review from waldyrious August 27, 2021 12:10
@Xymph Xymph force-pushed the 133-fix-exception-msg branch from 803366d to 1876cdd Compare August 27, 2021 13:11
Copy link
Copy Markdown
Collaborator

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

Just a minor suggestion for the changelog entry. Also, the commit message exceeds the recommended guideline for commit message length and indeed GitHub cuts off the message at the 70th character: see 1876cdd. Could you shorten it a bit?

Comment thread CHANGELOG.md Outdated
@Xymph Xymph force-pushed the 133-fix-exception-msg branch from 1876cdd to fba99aa Compare August 27, 2021 13:25
@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Aug 27, 2021

Just a minor suggestion for the changelog entry. Also, the commit message exceeds the recommended guideline for commit message length and indeed GitHub cuts off the message at the 70th character: see 1876cdd. Could you shorten it a bit?

Done, but I realize I'm being a bit hasty on this PR. Please don't merge it yet, it can simmer a few hours, or until tomorrow.

@Xymph Xymph force-pushed the 133-fix-exception-msg branch from fba99aa to 4c07250 Compare August 27, 2021 13:29
@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Aug 27, 2021

The backslash on the exception name specifies global namespace, something I'm not very familiar with yet. But I've come to understand this may better also be used on the 'new stdClass' added in #119. I hesitated to do that at the time.

However, because Wikimate itself doesn't define a namespace (yet?) there's no difference in practice. So it would be overkill to make a separate one-character PR for this. But before you hate me for sneaking that change into this PR 😆 I figured I better raise the topic first. Would you accept it in this case?

@Xymph Xymph requested a review from waldyrious August 27, 2021 13:37
@waldyrious
Copy link
Copy Markdown
Collaborator

Done, but I realize I'm being a bit hasty on this PR. Please don't merge it yet, it can simmer a few hours, or until tomorrow.

Sure :)

The backslash on the exception name specifies global namespace

Ah, I did wonder if that could have been a typo of some sort! 😄

it would be overkill to make a separate one-character PR for this. But before you hate me for sneaking that change into this PR 😆 I figured I better raise the topic first. Would you accept it in this case?

Hahah, no worries :) I think you've been quite measured with the snuck-in changes ;) I agree that a one-character change, to adopt a syntax that is already being introduced in this PR, is quite reasonable.

Comment thread USAGE.md Outdated
@Xymph Xymph force-pushed the 133-fix-exception-msg branch 2 times, most recently from c4e2ce5 to 7dffdc6 Compare August 27, 2021 20:49
@Xymph Xymph force-pushed the 133-fix-exception-msg branch from 7dffdc6 to 225b742 Compare August 27, 2021 20:51
@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Aug 28, 2021

Done, but I realize I'm being a bit hasty on this PR. Please don't merge it yet, it can simmer a few hours, or until tomorrow.

Other than the linefeeds topic, this PR is now good to go.

Copy link
Copy Markdown
Collaborator

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

LGTM!

@waldyrious waldyrious merged commit e5b1008 into hamstar:master Aug 29, 2021
@Xymph Xymph deleted the 133-fix-exception-msg branch August 29, 2021 17:38
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