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

Export GRAPHQL_MAX_INT and GRAPHQL_MIN_INT #3355

Merged
merged 3 commits into from
Nov 16, 2021

Conversation

tofran
Copy link
Contributor

@tofran tofran commented Nov 2, 2021

Closes #2524

(Replaces abandoned #2582)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 2, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

Please export it in src/type/index.ts and src/index.ts.
Also please check tests folder for the places where this number (or +-1) is used and replace them with this constant.

src/type/scalars.ts Outdated Show resolved Hide resolved
@tofran
Copy link
Contributor Author

tofran commented Nov 4, 2021

Hello @IvanGoncharov , thank you for bringing these up and your detailed comment.
I have made the requested changes:

  • Checked the tests folder for the places where this number (or +-1) is used and replace them with this constant.*
$ grep -R "214748364" src  # Intentionally missing the last character
src/type/scalars.ts:export const GRAPHQL_MAX_INT = 2147483647;
src/type/scalars.ts:export const GRAPHQL_MIN_INT = -2147483648;
$ grep -R "2^31" src
src/type/scalars.ts:    'The `Int` scalar type represents non-fractional signed whole numeric values. Int can represent values between -(2^31) and 2^31 - 1.',
$ grep -R "2 \*\* 31" src
# no results
  • Converted above comment to JSDoc: separated and simplified them.
  • Exported it in src/type/index.ts and src/index.ts.
  • Renamed constants to GRAPHQL_INT_MAX and GRAPHQL_INT_MIN.

*Were you also suggesting that tests to these boundaries should be changed?
Ex:

expect(() => parseLiteral('-9876504321')).to.throw(
  'Int cannot represent non 32-bit signed integer value: -9876504321',
);

Would now be

expect(() => parseLiteral(GRAPHQL_MIN_INT-1)).to.throw(
  `Int cannot represent non 32-bit signed integer value: ${GRAPHQL_MIN_INT-1}`,
);

Should I make this change? Thank you.

@tofran tofran requested a review from IvanGoncharov November 4, 2021 09:46
@tofran tofran changed the title Add exports to MAX_INT and MIN_INT in scalars.ts Export GRAPHQL_MAX_INT and GRAPHQL_MIN_INT Nov 4, 2021
@tofran tofran requested a review from saihaj November 4, 2021 11:08
@saihaj saihaj added the PR: feature 🚀 requires increase of "minor" version number label Nov 5, 2021
src/index.ts Outdated Show resolved Hide resolved
Matching the order from type/index.ts
@tofran
Copy link
Contributor Author

tofran commented Nov 7, 2021

@IvanGoncharov @saihaj thank you.
Just pushed the requested changes, ready to merge.

@tofran
Copy link
Contributor Author

tofran commented Nov 14, 2021

@IvanGoncharov I don't think we will be able to merge until you re-review.
Thanks in advance.

@IvanGoncharov IvanGoncharov merged commit 11a0802 into graphql:main Nov 16, 2021
@IvanGoncharov
Copy link
Member

@tofran Thanks 👍
Merged 🎉

Copy link

@jen801 jen801 left a comment

Choose a reason for hiding this comment

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

@graphql graphql locked as spam and limited conversation to collaborators Dec 12, 2021
@saihaj
Copy link
Member

saihaj commented Dec 12, 2021

Just to avoid noise from potential bot accounts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add max int to exports
5 participants