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

Add DefaultFormatter.DisableScientific #220

Merged

Conversation

rafiramadhana
Copy link
Contributor

@rafiramadhana rafiramadhana commented Jan 2, 2023

Issue: #190

Tasks:

  • Add DisableScientific flag in DefaultFormatter
  • Add/update sample unit test (for early review)
  • Add/update remaining unit test (if sample is ok)
  • Refine comments
  • Complete unit test with other assertion types (e.g. formatting of AssertionList with number as mentioned in Add DefaultFormatter.DisableScientific #220 (comment))

Update unit test of float32 on disable scientific.
@coveralls
Copy link
Collaborator

coveralls commented Jan 2, 2023

Coverage Status

Coverage: 94.435% (-0.1%) from 94.555% when pulling eb91b8a on neverbeenthisweeb:190-default-formatter-disable-scientific into 86f366a on gavv:master.

@gavv

This comment was marked as resolved.

formatter.go Outdated Show resolved Hide resolved
@gavv gavv added the work in progress Pull request is still in progress and changing label Jan 2, 2023
@gavv

This comment was marked as resolved.

@rafiramadhana

This comment was marked as resolved.

Update documentation of DisableScientific.
Separate DisableScientific unit test.
Remove unused fields in unit test.
@rafiramadhana

This comment was marked as resolved.

formatter.go Outdated Show resolved Hide resolved
formatter.go Outdated Show resolved Hide resolved
@gavv

This comment was marked as resolved.

gavv

This comment was marked as resolved.

@gavv gavv added the needs revision Pull request should be revised by its author label Jan 10, 2023
@rafiramadhana

This comment was marked as resolved.

@gavv

This comment was marked as resolved.

@gavv gavv removed the needs revision Pull request should be revised by its author label Jan 13, 2023
@rafiramadhana

This comment was marked as resolved.

@gavv

This comment was marked as resolved.

@rafiramadhana

This comment was marked as resolved.

formatter.go Outdated Show resolved Hide resolved
@gavv
Copy link
Owner

gavv commented Jan 21, 2023

The updated test looks good.

Also it would be nice to cover these cases:

  • formatting of AssertionList with number
  • formatting of Expected with number that is not AssertionList or AssertionRange
  • formatting of Actual with number
  • formatting of Reference with number

What else? I guess only #220 (comment) ?

@rafiramadhana

This comment was marked as resolved.

@rafiramadhana

This comment was marked as resolved.

Remove isFloat because extractFloat has the same responsibility.
With FloatFormat we can decide to print float in decimal, scientific notation, or auto.
Line too long.
@rafiramadhana

This comment was marked as resolved.

@rafiramadhana

This comment was marked as resolved.

@gavv

This comment was marked as resolved.

@gavv

This comment was marked as resolved.

formatter.go Outdated Show resolved Hide resolved
formatter.go Outdated Show resolved Hide resolved
formatter.go Outdated Show resolved Hide resolved
@gavv gavv force-pushed the master branch 2 times, most recently from 43b0a86 to ee27d9f Compare January 27, 2023 08:20
@rafiramadhana

This comment was marked as resolved.

Update comments related to FloatFormat.
@gavv

This comment was marked as resolved.

@gavv

This comment was marked as resolved.

@gavv

This comment was marked as resolved.

@rafiramadhana

This comment was marked as resolved.

@rafiramadhana

This comment was marked as resolved.

@rafiramadhana
Copy link
Contributor Author

this MR is now ready @gavv

@gavv gavv added ready for review Pull request can be reviewed and removed work in progress Pull request is still in progress and changing ready for review Pull request can be reviewed labels Feb 1, 2023
@gavv gavv marked this pull request as ready for review February 1, 2023 11:25
@gavv gavv merged commit 2cb5998 into gavv:master Feb 1, 2023
@gavv
Copy link
Owner

gavv commented Feb 1, 2023

Thanks!!

I've pushed a few follow-up commits:

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