Skip to content

Move KeyValueIterable and KeyValueIterableView from trace to common#2

Closed
kxyr wants to merge 1 commit into
masterfrom
move-KVI-KVIV
Closed

Move KeyValueIterable and KeyValueIterableView from trace to common#2
kxyr wants to merge 1 commit into
masterfrom
move-KVI-KVIV

Conversation

@kxyr
Copy link
Copy Markdown

@kxyr kxyr commented Oct 15, 2020

A simple PR based on this comment from @maxgolov.

Since KeyValueIterable is used by metrics, traces, and logs alike, it would be better if it belonged to the “common” namespace as it is no longer exclusive to being used for “trace”.

In this PR, KeyValueIterable and the derived class KeyValueIterableView are refactored from namespace trace::* to common::* and their corresponding *.h files are moved from api/.../trace to api/.../common.

The change continues to pass the key_value_iterable_view_test.cc, which runs unit tests for the abstract KeyValueIterable class and KeyValueIterableView inherited class, and does not break the build tests.

cc @alolita @MarkSeufert

@kxyr
Copy link
Copy Markdown
Author

kxyr commented Oct 15, 2020

Note: code coverage does not change

Copy link
Copy Markdown

@alolita alolita left a comment

Choose a reason for hiding this comment

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

lgtm. #shipit

@kxyr kxyr closed this Oct 16, 2020
@maxgolov
Copy link
Copy Markdown

@xukaren - Karen, did you guys actually merge it? Looks like it's closed by not merged?

@maxgolov
Copy link
Copy Markdown

oh I see, basically you are collecting fixes in your repo, then gonna do one big pull request into mainline?

@maxgolov
Copy link
Copy Markdown

@xukaren - absolutely not a problem. I actually appreciate tagging me, I would love to have an early peek at your work before you submit it for formal PR review in the mainline branch!

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