Change maxParallelSession type to Option<u8>
#383
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
=======================================
Coverage 71.33% 71.33%
=======================================
Files 67 67
Lines 14685 14685
=======================================
Hits 10476 10476
Misses 4209 4209 ☔ View full report in Codecov by Sentry. |
div-seungha
changed the title
maxParallelSession type to Option<u32>maxParallelSession type to Option<u32>
div-seungha
force-pushed
the
div/max-parallel-session-type-fix
branch
from
a5847bc to
2f123f2
Compare
div-seungha
changed the title
maxParallelSession type to Option<u32>maxParallelSession type to Option<i32>
div-seungha
changed the title
maxParallelSession type to Option<i32>maxParallelSession type to Option<i32>
|
Since the corresponding review-database has been updated, I think this PR should proceed. Could you do that? |
div-seungha
force-pushed
the
div/max-parallel-session-type-fix
branch
from
2f123f2 to
7572961
Compare
div-seungha
changed the title
maxParallelSession type to Option<i32>maxParallelSession type to Option<i32>
div-seungha
force-pushed
the
div/max-parallel-session-type-fix
branch
from
7572961 to
e1045b0
Compare
I have rebased on the latest main branch and replaced the previously used |
CHANGELOG.md
Outdated
| @@ -58,6 +58,10 @@ Versioning](https://semver.org/spec/v2.0.0.html). | |||
| set to an empty string were not saved to the `config` in the database. | |||
| - Fixed an issue where configuration conversion failures were silently ignored, | |||
| leading to incorrect None handling. | |||
| - Changed `maxParallelSession` in `Account` to `Option<i32>.` Previously, | |||
| `maxParallelSession` was returned as a `Option<StringNumber>` type. However, | |||
There was a problem hiding this comment.
- I think we need to mention that this changelog entry is related to GraphQL API.
- Is
maxParallelSessioninAccountchanged toOption<i32>.? - I think it would be beneficial to clarify that the field is within the u8 range.
There was a problem hiding this comment.
I have modified the changelog to contain the meaning more clearly.
|
In addition to mentioning the range in the Changelog, GraphQL documentation should be updated accordingly. |
div-seungha
force-pushed
the
div/max-parallel-session-type-fix
branch
3 times, most recently
from
2622565 to
0a0ec6c
Compare
Is it correct that the "GraphQL documentation" you mentioned means line 677? If correct, I pushed the revised commit. |
CHANGELOG.md
Outdated
| - The `maxParallelSession` field in `Account` has been updated to `Option<i32>`, | ||
| replacing the previous `Option<StringNumber>` type. In the review-database, |
There was a problem hiding this comment.
- The `maxParallelSession` field in `Account` has been updated to `Option<i32>`,
replacing the previous `Option<StringNumber>` type.
는 지칭하는 것이 잘못되었습니다. 올바르게 적어주세요.
CHANGELOG.md
Outdated
| leading to incorrect None handling. | ||
| - The `maxParallelSession` field in `Account` has been updated to `Option<i32>`, | ||
| replacing the previous `Option<StringNumber>` type. In the review-database, | ||
| the `max_parallel_sessions` type has been changed from `Option<u32>` to | ||
| `Option<u8>`, with the related code updated accordingly. Since GraphQL does | ||
| not support unsigned integers, the type has been replaced with the signed type | ||
| `Option<i32>`, which can accommodate the `Option<u8>` values. |
There was a problem hiding this comment.
유저 관점에서, 유저가 알아야 하는 정보를 적어주세요.
div-seungha
force-pushed
the
div/max-parallel-session-type-fix
branch
from
0a0ec6c to
c704d4a
Compare
CHANGELOG.md
Outdated
| - Updated the `maxParallelSessions` field in `Account` to Option<i32>, replacing | ||
| the previous `Option<StringNumber>` type. This change aligns with the update | ||
| in the review-database, where the `max_parallel_sessions` type was changed | ||
| from `Option<u32>` to `Option<u8>`. Since GraphQL does not support unsigned | ||
| integers, `Option<i32>` was chosen to ensure compatibility while accommodating | ||
| the `Option<u8>` values. |
There was a problem hiding this comment.
I think a quick sync meeting for this might help us, @div-seungha would be up for it for a second?
There was a problem hiding this comment.
I revised the commit considering we talked about in a separate communication channel.
div-seungha
force-pushed
the
div/max-parallel-session-type-fix
branch
2 times, most recently
from
b7e1894 to
62d4850
Compare
CHANGELOG.md
Outdated
| - The `account` and related queries such as `accountList` now return | ||
| `maxParallelSessions` as an `Int` within the range of `u8`. | ||
| - The `insertAccount` and `updateAccount` GraphQL APIs remain unchanged in | ||
| their interfaces but now only accept `maxParallelSessions` parameters within |
There was a problem hiding this comment.
Does updateAccount have maxParallelSessions parameter?
There was a problem hiding this comment.
but now only accept parameters related to max parallel sessions within the range of
u8.
No, It was my mistake. I have just modified the sentence.
div-seungha
force-pushed
the
div/max-parallel-session-type-fix
branch
from
62d4850 to
81a2a61
Compare
sehkone
changed the title
maxParallelSession type to Option<i32>maxParallelSession type to Option<u8>
Closes: #382