Skip to content

Order metadata by groupId#535

Open
michaeljs1990 wants to merge 1 commit intotumblr:masterfrom
michaeljs1990:lshw-ordering-bug
Open

Order metadata by groupId#535
michaeljs1990 wants to merge 1 commit intotumblr:masterfrom
michaeljs1990:lshw-ordering-bug

Conversation

@michaeljs1990
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

byxorna commented Apr 6, 2017

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

@michaeljs1990
Copy link
Copy Markdown
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