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

MessagePack deserialization: improve error messages #849

Merged
merged 5 commits into from
Dec 4, 2024
Merged

MessagePack deserialization: improve error messages #849

merged 5 commits into from
Dec 4, 2024

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Dec 3, 2024

No description provided.

@mgovers
fix error message when serializated file invalid
a38fb54 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers added the improvement Improvement on internal implementation label Dec 3, 2024
@mgovers mgovers self-assigned this Dec 3, 2024
figueroa1395
figueroa1395 previously approved these changes Dec 3, 2024
View reviewed changes
Jerry-Jinfeng-Guo
Jerry-Jinfeng-Guo previously requested changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

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

Please update the error messages:
either "A/An {item 1} or a/an {item 2} expected "
or
"Expecting a/an {item 1} or a/an {item 2}"

@TonyXiang8787
Copy link
Member

What are we fixing apart from the error messages?

@mgovers
Copy link
Member Author

mgovers commented Dec 3, 2024

What are we fixing apart from the error messages?

read as (fix + improve) error messages. will update

@mgovers mgovers changed the title MessagePack deserialization: fix + improve error messages MessagePack deserialization: improve error messages Dec 3, 2024
@mgovers
Update power_grid_model_c/power_grid_model/include/power_grid_model/a…
20f8276 
…uxiliary/serialization/deserializer.hpp


Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

Signed-off-by: Martijn Govers <martygovers@hotmail.com>
@mgovers
Copy link
Member Author

mgovers commented Dec 3, 2024

kept the Expected ... pattern. It doesn't look that weird if you read the full error
image

@Jerry-Jinfeng-Guo
Copy link
Contributor

kept the Expected ... pattern. It doesn't look that weird if you read the full error image

That's exactly what I couldn't agree...

@TonyXiang8787
Copy link
Member

I still do not get the purpose of this PR. The original error message is already very clear. The changes in this PR are only about the tense.

@mgovers
Copy link
Member Author

mgovers commented Dec 4, 2024

I still do not get the purpose of this PR. The original error message is already very clear. The changes in this PR are only about the tense.

In addition, while present tense was clear, it felt more like imperative tense (which it clearly isn't).

In this case, while we haven't dug into debugging yet, the JSON looked correct at first sight

  • so the error message became confusing: "expect an integer" > "it tells me to expect something?"
  • Past tense would have been more clear: "Expected an integer" > ah ok something went wrog. Maybe internally?

mgovers and others added 3 commits December 4, 2024 13:09
@mgovers
apply suggestions from code review
a7b0185 
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Merge branch 'feature/error-message-fix' of github.com:PowerGridModel…
0d9c5e5 
…/power-grid-model into feature/error-message-fix

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Merge branch 'main' into feature/error-message-fix
964abd2
@mgovers mgovers dismissed Jerry-Jinfeng-Guo’s stale review December 4, 2024 12:19

tony approved but this requested changes needs to be dismissed before PR can be merged

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Dec 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@TonyXiang8787 TonyXiang8787 added this pull request to the merge queue Dec 4, 2024
Merged via the queue into main with commit 0139a97 Dec 4, 2024
28 checks passed
@TonyXiang8787 TonyXiang8787 deleted the feature/error-message-fix branch December 4, 2024 13:33
@nitbharambe nitbharambe mentioned this pull request Jan 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TonyXiang8787 TonyXiang8787 TonyXiang8787 approved these changes

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo approved these changes

@figueroa1395 figueroa1395 figueroa1395 left review comments

Assignees

@mgovers mgovers

Labels
improvement Improvement on internal implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mgovers @TonyXiang8787 @Jerry-Jinfeng-Guo @figueroa1395