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

[Rust] Match integer type to minimum and maximum constraints #6985

Merged
merged 1 commit into from
Dec 10, 2017
Merged

[Rust] Match integer type to minimum and maximum constraints #6985

merged 1 commit into from
Dec 10, 2017

Conversation

fralalonde
Copy link
Contributor

@frol @farcaller

In rust-server, use rust integer types best matching the format, minimum and maximum schema constraints.

Matching rules cover all rust standard int size, from 8 to 64bits, signed and unsigned, including size types.

  • minimum is used to determine if the int type should be signed. Default is signed.
  • maximum is used to determine the int size. Default is isize or usize.
  • Standard formats int32 and int64 can be used to force int size.
  • Existing custom formats uint32 and uint64 have been preserved, even though they are superseded by new code.
  • minimum and maximum constraints also respects their exclusive flags if specified.

@jaytiar
Copy link

jaytiar commented Nov 17, 2017

The logic as you've described it (assuming I've understood) doesn't seem to cope with cases where the valid range is -(something large) to +(something small). You'll correctly pick a signed quantity, but the size will only be big enough for the positive max, not the negative max. I think you need to use the maximum and minimum to determine the size needed.

@fralalonde
Copy link
Contributor Author

You are right. I changed the calculation to use Long.numberOfLeadingZeros to determine the required int size (acounting signedness), which should cover that case.

As a sensible default, I made usize and isize be used when the minimum is unspecified or requires less than 32 bits to be handled.

I also added a test for the min / max constraints to int sizing logic, you can look at it, it makes the choices clearer.

@wing328
Copy link
Contributor

wing328 commented Nov 27, 2017

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/swagger-api/swagger-codegen/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@fralalonde
Copy link
Contributor Author

fralalonde commented Nov 27, 2017

Thank you for noticing this. I fixed my local git config for the future, amended the commit and added my secondary email to my github account for good measure.

@fralalonde
Copy link
Contributor Author

Although the commit is now to my primary email, I noticed that my avatar picture for the commit is small, and the generic octocat is still shown behind. Let me know if this is normal or if it's something I should fix.

Copy link
Contributor

@bjgill bjgill left a comment

Choose a reason for hiding this comment

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

One minor typo. Otherwise looks good.

assertEquals(RustServerCodegen.matchingIntType(false, null, Long.MAX_VALUE), "i64");
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@@ -269,7 +262,7 @@ public String toApiName(String name) {

/**
* Escapes a reserved word as defined in the `reservedWords` array. Handle escaping
* those terms here. This logic is only called if a variable matches the reserved words
* those terms here. This logic is only called if a variable matches the reseved words
Copy link
Contributor

Choose a reason for hiding this comment

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

reserved?

@wing328 wing328 merged commit f49109c into swagger-api:master Dec 10, 2017
@wing328
Copy link
Contributor

wing328 commented Dec 10, 2017

@fralalonde thanks for the PR, which has been merged into master.

@bjgill thanks for reviewing the change.

@wing328 wing328 changed the title Match rust integer type to minimum and maximum constraints [Rust] Match integer type to minimum and maximum constraints Dec 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants