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

Order metadata by groupId #535

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Order metadata by groupId #535

wants to merge 1 commit into from

Conversation

michaeljs1990
Copy link
Contributor

@michaeljs1990 michaeljs1990 commented Mar 26, 2017

This orders metadata by the groupId of that metadata before passing it
onto other functions for processing. Since a Map was previously used you
could end up with your data being rendered in any which order. Now data
provided you map it correctly in the following functions will be displayed
in the proper order.

TODO:

  • write some tests that verify all reconstructing* functions do this properly.

Report: #534

This orders metadata by the groupId of that metadata before passing it
onto other functions for processing. Since a Map was previously used you
could end up with your data being rendered in any which order. Now data
provided you map it correctly in the following functions will be displayed
in the proper order.

Report: #534
@michaeljs1990
Copy link
Contributor Author

All tests still pass but someone else should look at this as i'm not sure if it could have other subtle effects.

@byxorna
Copy link
Contributor

byxorna commented Mar 28, 2017

This LGTM, but I would definitely like unit tests to ensure expected behavior. Can you contrive a fixture lshw/lldp that illustrates the behavior?

@byxorna
Copy link
Contributor

byxorna commented Apr 6, 2017

@michaeljs1990 bump, checking on unit tests. Is this merge blocking anything internal for you?

@michaeljs1990
Copy link
Contributor Author

No this one is not. I was actually just bored that night so I tried to fix the issue in #534 since it sounded interesting. I'll likely get some unit tests added for this over the weekend.

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.

2 participants