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

fix : noneprefixer did not return correct fixed len #304

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

T-eli
Copy link
Contributor

@T-eli T-eli commented Jan 16, 2024

this pull request contains 2 fixes:

  1. noneprefixer fixed was returning len(data) instead of fixed value
  2. text edit: numeric field parses int64 not int

while i am here i have a question if the someone can answer:
-> Aren't Numeric Fields usually encoded in BCD?

Nemric field pack method: data := []byte(strconv.FormatInt(f.value, 10))...

should be : data, _ := hex.DecodeString(strconv.FormatInt(f.value, 10))

I know that you can set the Encoder in spec to BCD. but I dont see the deference between using a String Field and Numeric Field.

@T-eli T-eli requested a review from alovak as a code owner January 16, 2024 10:17
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (27feb74) 74.98% compared to head (87a4dcb) 74.98%.

❗ Current head 87a4dcb differs from pull request most recent head 09e2fc7. Consider uploading reports for the commit 09e2fc7 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #304   +/-   ##
=======================================
  Coverage   74.98%   74.98%           
=======================================
  Files          44       44           
  Lines        2495     2495           
=======================================
  Hits         1871     1871           
  Misses        407      407           
  Partials      217      217           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alovak
Copy link
Contributor

alovak commented Jan 16, 2024

Hey! Thanks for the PR and the fixes!

while i am here i have a question if the someone can answer: -> Aren't Numeric Fields usually encoded in BCD?

Nemric field pack method: data := []byte(strconv.FormatInt(f.value, 10))...

should be : data, _ := hex.DecodeString(strconv.FormatInt(f.value, 10))

I know that you can set the Encoder in spec to BCD. but I dont see the deference between using a String Field and Numeric Field.

I saw different encodings used for the numeric fields. It depends on the card brand / processor.

The difference between String and Numeric is in the type of the underlying value string vs int64:

str := field.NewStringValue("100")
str.Value() // "100"

i := field.NewNumericValue(100)
i.Value() // 100

There will be an error if you try to unpack a message with a non-numeric field value for a field specified as field.Numeric.

prefix/none.go Outdated Show resolved Hide resolved
@alovak
Copy link
Contributor

alovak commented Jan 16, 2024

We should revert the change to the None prefixer before we can merge the fixes.

@T-eli
Copy link
Contributor Author

T-eli commented Jan 16, 2024

hello @alovak ,

thank you for answering my question.

i was confused by the none.fixed which i interpreted as no prefix + use fixed length

anyways i cherry picked to other changes. if you want to merge this PR

Lyes added 2 commits January 16, 2024 12:24
Copy link
Contributor

@alovak alovak left a comment

Choose a reason for hiding this comment

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

@T-eli thanks a lot for the fixes!

@alovak alovak merged commit e656fa3 into moov-io:master Jan 16, 2024
8 checks passed
@alovak
Copy link
Contributor

alovak commented Jan 16, 2024

@T-eli if you have any other questions, feel free to ask them on either https://github.com/moov-io/iso8583/issues or joining Moov Slack community (channel #iso8583)

@T-eli
Copy link
Contributor Author

T-eli commented Jan 16, 2024

thanks @alovak.

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.

None yet

3 participants