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

Namespace for classification types #11

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Conversation

stefpap
Copy link
Contributor

@stefpap stefpap commented Aug 13, 2022

This PR addresses issue #8

  • Added namespaces for classification types
  • Added requestDocs & requestTransmittedDocs pagination parameters

@stefpap stefpap requested a review from ppapapetrou76 as a code owner August 13, 2022 12:02
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2022

Codecov Report

Merging #11 (2f173a3) into main (2b3ab75) will decrease coverage by 0.97%.
The diff coverage is 90.69%.

@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
- Coverage   93.70%   92.72%   -0.98%     
==========================================
  Files           3        5       +2     
  Lines         127      165      +38     
==========================================
+ Hits          119      153      +34     
- Misses          4        6       +2     
- Partials        4        6       +2     
Impacted Files Coverage Δ
pkg/models/expense_classification.go 80.00% <80.00%> (ø)
pkg/models/income_classification.go 80.00% <80.00%> (ø)
pkg/mydata/client.go 93.44% <100.00%> (+1.13%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment on lines 24 to 41

func (classification ExpensesClassificationType) MarshalXML(enc *xml.Encoder, start xml.StartElement) error {
type expensesClassificationType struct {
ClassificationType string `xml:"ecls:classificationType"`
ClassificationCategory string `xml:"ecls:classificationCategory"`
Amount float64 `xml:"ecls:amount"`
ID *byte `xml:"ecls:id"`
}

err := enc.EncodeElement(expensesClassificationType(classification), start)
if err != nil {
return fmt.Errorf("xml marshal expense classification: %w", err)
}

return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice i made a new duplicate struct of the ExpenseClassificationType here with just the namespace prefix added ecls:.
The reason for this is because I came across an issue when marshaling/unmarshaling ExpensesClassificationType &. IncomeClassificationType xml with namespaces and then trying to retrieve them with RequestDocs
golang/go#11496
golang/go#9519

Comment on lines +25 to +41
func (classification IncomeClassificationType) MarshalXML(enc *xml.Encoder, start xml.StartElement) error {
type incomeClassificationType struct {
ClassificationType string `xml:"icls:classificationType"`
ClassificationCategory string `xml:"icls:classificationCategory"`
Amount float64 `xml:"icls:amount"`
ID *byte `xml:"icls:id"`
}

err := enc.EncodeElement(incomeClassificationType(classification), start)
if err != nil {
return fmt.Errorf("xml marshal income classification: %w", err)
}

return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as ExpensesClassificationType reasoning

Copy link
Owner

@ppapapetrou76 ppapapetrou76 left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!! - could you please consider adding a few UTs ?

@stefpap
Copy link
Contributor Author

stefpap commented Aug 28, 2022

Coverage looking better after adding some test cases. Let me know what you think : )

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@stefpap
Copy link
Contributor Author

stefpap commented Sep 12, 2022

I just noticed I had to sign all of my changes so I squashed them as well. No new changes introduced in todays commit.

Also I get the message First-time contributors need a maintainer to approve running workflows

@ppapapetrou76 ppapapetrou76 merged commit 09e68b9 into ppapapetrou76:main Oct 12, 2022
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