Add DefaultFormatter.EnableScientific

[gavv]

Add flag EnableScientific to DefaultFormatter that enables printing floats in scientific form. Without this flag, floats should be always printed in non-scientific form.

See also #160 and #159.

[rafiramadhana]

hi @gavv , can I work on this? thanks

[gavv]

Sure!

[rafiramadhana]

thanks

will make progress within this week

[rafiramadhana]

hi @gavv

in httpexpect we have this formatFloat function, which is called by filDelta:

httpexpect/formatter.go

Line 394 in 0bdec41

data.Delta = formatFloat(failure.Delta.Value)

question: is the scope of this change only within "fillDelta that calls formatFloat"? or it could be bigger?

because it looks like formatRange and isNumber also involves float64 type

thanks

[gavv]

Good question.

formatFloat is used only for printing delta when use use assertions like EqualDelta. This feature is definitely broader than just printing deltas, it should apply to actual and expected values in all assertion types.

Actually I don't quite understand why I added formatFloat. It seems misleading, because most times it's not used for formatting floats..

I think we should remove formatFloat at all and instead use formatValue and teach formatValue to print floats as formatFloat does it.

Also, as you correctly noticed, numbers may be printed via formatRange. Let's rework formatRange so that it will use formatValue to format min and max; this way all float formatting logic will be in one place - in formatValue.

Similarly, we should rework formatTyped to use formatValue when value is a number.

[gavv]

Also, after more thought, it seems that it's better to reverse the option: add DisableScientific instead of EnableScientific.

It would work as follows:

• if DisableScientific is false (default), use %g to format floats (which will automatically use scientific notation for large exponents); currently behavior is the same for everything except deltas (which use formatFloat) - go automatically uses %g when you use %v with floats

• if DisableScientific is true, use strconv.FormatFloat with prec set to -1; this will format float in non-scientific notation with number of digits set to minimum number of digits needed for exact representation (see https://pkg.go.dev/strconv#FormatFloat and https://cs.opensource.google/go/go/+/refs/tags/go1.19.4:src/strconv/ftoa.go;l=113;drc=8bcc490667d4dd44c633c536dd463bbec0a3838f)

[rafiramadhana]

this way all float formatting logic will be in one place - in formatValue.

ok, noted

so formatValue will be the entry point for any formatting (integer, float, string, etc) CMIIW

[rafiramadhana]

Also, after more thought, it seems that it's better to reverse the option: add DisableScientific instead of EnableScientific.

agree

btw it looks like we don't have unit test for DefaultFormatter (i think it is important to test the flags), or i am missing something?

[rafiramadhana]

most of the tests in formatter_test.go directly test the formatting (e.g formatValue and formatRange)

[rafiramadhana]

maybe we can test the DefaultFormatter.buildFormatData and assert the templateData

httpexpect/formatter.go

Line 141 in 0bdec41

templateData := f.buildFormatData(ctx, failure)

IMO on the contrary...if we assert the formatTemplate, it might be tedious because perhaps there will be a lot of text from the template result

httpexpect/formatter.go

Line 134 in 0bdec41

func (f *DefaultFormatter) formatTemplate(

wdyt?

[gavv]

so formatValue will be the entry point for any formatting (integer, float, string, etc) CMIIW

Yes.

btw it looks like we don't have unit test for DefaultFormatter (i think it is important to test the flags), or i am missing something?

We have a basic test in TestE2EReportNames, and an issue for expanding them: #165

However they are e2e tests that cover formatter very shallow and indirectly (thought I still think they would be helpful anyway).

maybe we can test the DefaultFormatter.buildFormatData and assert the templateData

This sound like a very good idea, I didn't think about it. And such tests would be extra valuable since FormatData is part of public API and currently it's not covered at all.

[rafiramadhana]

However they are e2e tests that cover formatter very shallow and indirectly (thought I still think they would be helpful anyway).

it looks like #165 idea is (quite) similar with asserting the formatTemplate

yea, maybe asserting the formatTemplate is preferable in e2e because that is what our user see (the formatted text)

[rafiramadhana]

ok, i think i will do these changes in order

1. update formatValue to be the entry point for any formatting (integer, float, string, etc)
2. add unit test implementation (perhaps using the templateData)
3. implement DisableScientific
4. re-evaluate unit test implementation (let's see if we found a better way to do assertion on this)

i will start the PR(s) as draft

thanks

[gavv]

Great, sounds good.

Yes, I think it makes sense to add both set of tests - described in #165 that match resulting text, and what you described here that match FormatData.

First set of tests will check the end result that is presented to user and will cover template formatter as well. Second will be more low-level and more granular, and wont bother about templates.

After completing this issue we may create a new one for adding more formatter unit tests based on FormatData.

[rafiramadhana]

got it

[rafiramadhana]

@gavv Hi, I just realised this.

How do we pass the DisabledScientific flag from DefaultFormatter to the formatValue?

type DefaultFormatter struct {
	// If true, format float in non-scientific notation
	DisableScientific bool

Maybe, we can add a new parameter to formatValue

Something like:

type formatValueConfig struct {
    DisableScientific bool
}

func formatValue(cfg formatValueConfig, value interface{}) string {

Btw, this idea is referencing this statement:

this way all float formatting logic will be in one place - in formatValue.

#190 (comment)

Wdyt? Thanks

[gavv]

We could make all formatXxx methods of formatter.

Why:

• they are used only in formatter
• formatter itself is actually "formatting config"; it has no other fileds except configuration and doesn't have state

[gavv]

Happy new year btw :)

[rafiramadhana]

ok, it looks like that would result to a lot of changes

but it is ok because we have the time

i will create a draft PR first, consisting:

• update all formatXXX to be a method of DefaultFormatter
• add unit test related to DisableScientific

[rafiramadhana]

happy new year to all of httpexpect contributors 🚀

[rafiramadhana]

This issue needs additional work of adding unit test

Complete unit test with other assertion types (e.g. formatting of AssertionList with number as mentioned in #220 (comment))

ref: #220 (comment)

[gavv]

This issue needs additional work of adding unit test

Fixed in d9300b9

[gavv]

Everything is merged

[gavv]

I've created a follow-up issue: #258

[gavv]

Two more follow-up issues: #262, #264