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 list conversion issues in the xml gatherer #157

Merged
merged 3 commits into from
Dec 23, 2022

Conversation

arbulu89
Copy link
Contributor

During the last sprint review I noticed that in some corner cases, the conversion to list was doing something strange.
At the end I have been forced to change how it was, adding some recursive code to convert the desired fields to list properly.

The code might looks complex, what it is what is is in this sort of things on golang...

@arbulu89 arbulu89 added the bug Something isn't working label Dec 19, 2022
@arbulu89 arbulu89 marked this pull request as ready for review December 20, 2022 08:49
@CDimonaco
Copy link
Member

I understand the complexity of the task and golang is really verbose, could we add some comments through the function? Just to have a better understanding of whats'happening? Due to all the casting I don't think we can achieve a really better refactoring, so the comments imho are the best way to address this right now :D

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Did you say interface{}? 😄

LGTM. If some comment help, as @CDimonaco is suggesting, it'd be ok for me.

Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

Brackets on the case and comments and we are good to go, nothing on the code itself but only on readability and future maintenance

internal/factsengine/gatherers/xml.go Outdated Show resolved Hide resolved
@arbulu89 arbulu89 requested a review from CDimonaco December 23, 2022 09:06
@arbulu89 arbulu89 merged commit 4c34887 into main Dec 23, 2022
@arbulu89 arbulu89 deleted the fix-cibadmin-list-conversion branch December 23, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants