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

OriginHosts and OriginNodes become Origins #194

Merged
merged 1 commit into from
Jun 9, 2015
Merged

OriginHosts and OriginNodes become Origins #194

merged 1 commit into from
Jun 9, 2015

Conversation

peterbourgon
Copy link
Contributor

Another baby step towards #123, this change follows from #192 and merges the two concepts of Origin in a renderable node. We also cut out a layer of abstraction, and add an OriginTable method to Report, which directly generates a table of info for the detail pane given any origin node ID.

// multiple origins. The ultimate goal here is to generate tables to view
// in the UI, so we skip the intermediate representations, but we could
// add them later.
outer:

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@tomwilkie tomwilkie assigned peterbourgon and unassigned tomwilkie Jun 9, 2015
@peterbourgon
Copy link
Contributor Author

How's that look?

@@ -1,94 +1 @@
package main

This comment was marked as abuse.

This comment was marked as abuse.

@tomwilkie
Copy link
Contributor

One last comment; otherwise LGTM

Is coveralls being weird or did it really drop by 6%? That might be worth investigating.

@tomwilkie
Copy link
Contributor

Also squash pls!

@peterbourgon
Copy link
Contributor Author

Is coveralls being weird or did it really drop by 6%? That might be worth investigating.

Coveralls has been being weird all day, but it wouldn't surprise me if coverage has dropped — there are a lot of tests that I'm waiting to port from #123, because they rely on features that haven't yet been moved over, and I'd rather do it all in one motion than waste time on getting them working in the intermediate steps and then blowing it away.

At the end of all of this, I'll promise a significant increase in test coverage, probably double-digit.

Another baby step towards #123, this change follows from #192 and merges
the two concepts of Origin in a renderable node. We also cut out a layer
of abstraction, and add an OriginTable method to Report, which directly
generates a table of info for the detail pane given any origin node ID.

Other changes from feedback:

- Assume origin IDs are unique and don't reflect.Dedupe
- Improve origin ID lookup
- Move OriginTable to detailed_node.go, as a free function
- rm app/detail_pane.go (empty)
@tomwilkie
Copy link
Contributor

SGTM!

peterbourgon added a commit that referenced this pull request Jun 9, 2015
OriginHosts and OriginNodes become Origins
@peterbourgon peterbourgon merged commit ff14a61 into master Jun 9, 2015
@peterbourgon peterbourgon deleted the origins branch June 9, 2015 16:58
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