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

Rename SpanContext to SpanReference #362

Closed
ThomsonTan opened this issue Oct 14, 2020 · 22 comments
Closed

Rename SpanContext to SpanReference #362

ThomsonTan opened this issue Oct 14, 2020 · 22 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@ThomsonTan
Copy link
Contributor

Update needed to track these changes: open-telemetry/opentelemetry-specification#1075

@ThomsonTan ThomsonTan added bug Something isn't working good first issue Good for newcomers labels Oct 14, 2020
@TuckTuckFloof
Copy link

Is this issue still open?

@ThomsonTan
Copy link
Contributor Author

@TuckTuckFloof yes, are you interested to help this?

@TuckTuckFloof
Copy link

Yea, I've been looking to do some open source stuff, complete first timer, now what do I have to do?

@ThomsonTan
Copy link
Contributor Author

Take a look at the spec link open-telemetry/opentelemetry-specification#1075, make sure all the occurrences are correctly renamed in this repo, and follow our contribution guideline here.

@TuckTuckFloof
Copy link

I'm in, so look for more instances like the one in the spec link and make a pr then?

@ThomsonTan
Copy link
Contributor Author

That's right. Just a reminder, please also take a look at the contribution guideline above before uploading PR. Thanks and happy contributing.

@TuckTuckFloof
Copy link

Thank you, and will do. I've already forked it and found 1 instance lol.

@TuckTuckFloof
Copy link

Oh, I found span_context, would you like me to change those to span_reference for consistency?

@ThomsonTan
Copy link
Contributor Author

That's a good find. I think it is great to keep it consistent with the struct name. @open-telemetry/cpp-approvers please share thoughts.

@TuckTuckFloof
Copy link

Alright, I got it done, I'm gonna hang around before the PR though, just in case you guys want me to change span_context to span_reference, as stated before

@TuckTuckFloof
Copy link

@ThomsonTan Do you think I should just make a PR as is or should I wait for other responses?

@ThomsonTan
Copy link
Contributor Author

I think it is fine to create the PR.

@TuckTuckFloof
Copy link

you got it

@TuckTuckFloof
Copy link

Ok, so I couldn't get the tools/format.sh file to work, couldnt find buildifier and the link to get it just doesn't exist apparently

@lalitb
Copy link
Member

lalitb commented Oct 15, 2020

Make sure you install formatting tool by running this script.

@TuckTuckFloof
Copy link

Oh, thank you, I hope this'll get it to work

@TuckTuckFloof
Copy link

TuckTuckFloof commented Oct 15, 2020

Alright, commited.

Edit: I don't think it PR'd, so we can solve this in the morning, because I couldn't get it to pr for some reason.

@TuckTuckFloof
Copy link

Alright, I'm back, it's telling me that considering that they are different commit histories, I can't make a PR

@ThomsonTan
Copy link
Contributor Author

Could you please share the detailed error message of creating the PR? Could you please try rebase your change to the latest and create PR again?

@TuckTuckFloof
Copy link

I'll try the rebase because it literally doesn't say anything else other than that they're different commit histories. Which is weird because it shows me the changes I made.

@lalitb
Copy link
Member

lalitb commented Oct 21, 2020

Please hold on this change, as there is ongoing discussion to revert back this renaming:

open-telemetry/opentelemetry-specification#1126

@lalitb
Copy link
Member

lalitb commented Oct 29, 2020

@ThomsonTan closing this issue -as the changes in specs are reverted back - open-telemetry/opentelemetry-specification#1127.

@lalitb lalitb closed this as completed Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants