Refactor formatValue

[rafiramadhana]

Issue: (part of) #190

Refactoring before proceeding to introduce DefaultFormatter.DisableScientific as mentioned in #190 (comment).

Refactoring (more detail #190 (comment)):

• Delete formatFloat. Replace with formatValue
• Rework formatTyped to use formatValue
• Rework formatRange to use formatValue
• Introduce scientific as default float format

Tasks:

• Add unit test to catch potential breaking early
• Refactor
• Re-evaluate unit test

PR will be draft, unless all tasks are checked

PR is now ready

[rafiramadhana]

hi @gavv , I've added unit test to guard from potential breaking when refactoring the formatValue

would you mind to take a look? thanks

[coveralls]

Coverage: 95.302% (+0.03%) from 95.275% when pulling ab04aae on neverbeenthisweeb:190-enable-scientific-formatter into 08a8b63 on gavv:master.

[gavv]

Nice!

Tests look good, but I thought you were going to create tests based on FormatData instead of of format template?

My understanding of the discussion was that template-based tests will be e2e tests covered by #165.

[rafiramadhana]

Nice!

Tests look good, but I thought you were going to create tests based on FormatData instead of of format template?

My understanding of the discussion was that template-based tests will be e2e tests covered by #165.

i see, sry

i thought it would be:

• e2e: format template
• unit test: FormatData + format template

ok, will adjust accordingly, so that:

• e2e: format template
• unit test: FormatData

[rafiramadhana]

updated

• Remove underscore in test names
• Test buildFormatData instead of template string

[rafiramadhana]

@gavv hi, may i know what is the intention of isNumber?

i think it is a bit vague when we see the existing formatFloat:

func formatFloat(value interface{}) string {
	switch value.(type) {
	case float32, float64:
		return fmt.Sprintf("%f", value)
	}

	if isNumber(value) {
		return fmt.Sprintf("%v", value)
	}

	return formatValue(value)
}

float32 and float64 are also numbers right? why we differ them from isNumber's check?

[rafiramadhana]

@gavv and maybe about formatValue

is it safe for me to assume in formatValue:

httpexpect/formatter.go

Line 401 in 4e92716

func formatValue(value interface{}) string {

that this part

if s, _ := value.(fmt.Stringer); s != nil {
	if ss := s.String(); strings.TrimSpace(ss) != "" {
		return ss
	}
}

has the same intention as formatString?

httpexpect/formatter.go

Line 420 in 4e92716

func formatString(value interface{}) string {

[gavv]

@gavv hi, may i know what is the intention of isNumber?

Intention was to use %f for floats and %v for other numbers (i.e. integers).

[gavv]

is it safe for me to assume in formatValue:
has the same intention as formatString?

IIRC they're different.

formatString is for printing bare strings. If value is string, it is printed as is, without adding quotes or anything. If it's not string, there is a fallback that uses formatValue. Other methods, e.g. formatRange, formatList, have similar fallbacks. Maybe it makes sense to rename them to formatStringOrValue, formatRangeOrValue, formatListOrValue.

formatValue is for printer arbitrary objects. It prints most values as JSON, with a few exceptions. If value is http.*, it prints it using litter. If value implements Stringer interface, i.e. it's an object with String() method, it uses that method to format object. It allows to pass values with custom formatting representation, e.g. see wsMessageType and wsCloseCode.

[gavv]

Note that when you pass a string to formatValue, it will select json.Marshal branch and print the string with quotes (this is how JSON represents strings). And if you pass string to formatString, it will print it without quotes.

Maybe it makes sens to rename formatString to formatBareString (or formatBareStringOrValue) to make this detail clearer.

[gavv]

We could also rename formatTyped to formatValueWithType.

[rafiramadhana]

@gavv ok thanks for the feedbacks

will revisit this pr either this weekend/next week

sry for late responses

[gavv]

Great, thanks!

[rafiramadhana]

@gavv Hi, this MR is now ready for review.

Maybe it makes sense to rename them to formatStringOrValue, formatRangeOrValue, formatListOrValue.

However, I haven't done those renaming changes. I think the OrValue is a bit vague about what meaning that it conveys. In addition, it is more readable without the OrValue IMO.

We could also rename formatTyped to formatValueWithType.

I also haven't done this change because I think it shows that formatTyped (which later renamed to formatValueWithType) has the same logic with formatValue.

I think, they are different because formatTyped uses %#v, while formatValue uses %v. Wdyt?

At the end, discussion is very welcome. We can do the changes here, or it could also be in another MR.

[rafiramadhana]

Btw, the e2e test will be done in another MR, can also have different issue for that.

I think, we can focus on the unit tests first + impl of .DisableScientific.

[gavv]

I think the OrValue is a bit vague about what meaning that it conveys. In addition, it is more readable without the OrValue IMO.

I also haven't done this change because I think it shows that formatTyped (which later renamed to formatValueWithType) has the same logic with formatValue.

Makes sense. Let's keep it as is, then.

I think, we can focus on the unit tests first + impl of .DisableScientific.

Agree.

[gavv]

Thanks for update! A few more comments.

[rafiramadhana]

@gavv ready for review

[gavv]

Awesome, thanks!