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

Allow noncompact CompactDecimal #2889

Merged
merged 10 commits into from
Dec 16, 2022
Merged

Conversation

eggrobin
Copy link
Member

As discussed with @sffc; towards #1267.

@eggrobin eggrobin requested a review from sffc as a code owner December 14, 2022 14:35
utils/fixed_decimal/src/compact.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/compact.rs Outdated Show resolved Hide resolved
utils/fixed_decimal/src/compact.rs Outdated Show resolved Hide resolved
@sffc
Copy link
Member

sffc commented Dec 14, 2022

Also please add docs to the two new functions

eggrobin and others added 4 commits December 14, 2022 16:50
Co-authored-by: Shane F. Carr <shane@unicode.org>
Co-authored-by: Shane F. Carr <shane@unicode.org>
Co-authored-by: Shane F. Carr <shane@unicode.org>
@eggrobin eggrobin requested a review from sffc December 14, 2022 16:11
sffc
sffc previously approved these changes Dec 14, 2022
@sffc
Copy link
Member

sffc commented Dec 15, 2022

Bumping CI

@sffc sffc closed this Dec 15, 2022
@sffc sffc reopened this Dec 15, 2022
@sffc
Copy link
Member

sffc commented Dec 15, 2022

[cargo-make] ERROR - Error while running duckscript: Source: Unknown Line: 6 - Diplomat coverage dump need to be regenerated. Please run `cargo make diplomat-coverage` and commit.

Manishearth
Manishearth previously approved these changes Dec 16, 2022
@@ -92,10 +136,6 @@ fn test_compact_syntax_error() {
pub expected_err: Option<Error>,
}
let cases = [
TestCase {
Copy link
Member

Choose a reason for hiding this comment

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

nit: probably add some testcases that ensure it works now

Copy link
Member Author

Choose a reason for hiding this comment

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

They are in the doc tests (and they even caught a hint bug); the #[test] only covers errors.

Copy link
Member

Choose a reason for hiding this comment

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

ah neat

@eggrobin eggrobin merged commit 8587c00 into unicode-org:main Dec 16, 2022
@robertbastian robertbastian mentioned this pull request Jan 23, 2023
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.

3 participants