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.

Sign up for GitHub

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

Jump to bottom

Use consistent file name for data-model.md #2586

Merged
merged 2 commits into from
Jun 13, 2022
Merged

Use consistent file name for data-model.md #2586

merged 2 commits into from
Jun 13, 2022

Conversation

tigrannajaryan
Copy link
Member

We use (mostly) snake-case for file names in the repository.
I renamed datamodel.md to data-model.md to be consistent.

@tigrannajaryan tigrannajaryan requested review from a team May 27, 2022 16:19
reyang
reyang approved these changes May 27, 2022
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

arminru
arminru reviewed May 31, 2022
View reviewed changes
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

Do you think we might have any incoming links that would break?

@arminru
Copy link
Member

arminru commented May 31, 2022

@chalin are there any changes that would need to be reflected on the website following this renaming?

@reyang reyang added the editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. label May 31, 2022
@carlosalberto
Copy link
Contributor

LGTM (prior to a review from @chalin as it may impact the docs in the OTel website).

Other than that, we should have an 'official' suggestion somewhere saying "this-is-my-file.md" format is preferred (and also consider updating other related items, e.g. should semantic_conventions/ become semantic-conventions/?

@tigrannajaryan
Copy link
Member Author

Do you think we might have any incoming links that would break?

It is possible. I am not sure there is a way to prevent it.

@tigrannajaryan
Use consistent file name for data-model.md
10ec7ab 
We use (mostly) snake-case for file names in the repository.
I renamed datamodel.md to data-model.md to be consistent.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/metric-data-model-name branch from e035558 to 10ec7ab Compare May 31, 2022 22:11
@reyang
Copy link
Member

reyang commented Jun 2, 2022

Do you think we might have any incoming links that would break?

It is possible. I am not sure there is a way to prevent it.

One possible way is to keep the existing file but shrink it to:

This file has been moved to ...

Anyways I think we shouldn't try to solve this problem. GitHub has permanent links, if folks use a non-permanent link, the default expectation is that it could break.

@tigrannajaryan
Copy link
Member Author

@chalin can you please confirm that we are not breaking anything at opentelemetry.io by this renaming?

@reyang reyang requested a review from chalin June 11, 2022 02:21
@reyang
Copy link
Member

reyang commented Jun 13, 2022

@chalin can you please confirm that we are not breaking anything at opentelemetry.io by this renaming?

@chalin looks like you are Away/OOO. Will merge the PR now, please follow up after you're back, thanks!

@reyang reyang merged commit bff2cc1 into open-telemetry:main Jun 13, 2022
@dmathieu dmathieu mentioned this pull request Jun 14, 2022
Merged
@tigrannajaryan tigrannajaryan deleted the feature/tigran/metric-data-model-name branch July 11, 2022 21:16
beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
@tigrannajaryan @beeme1mr
Use consistent file name for data-model.md (open-telemetry#2586)
3e96900
@chalin chalin mentioned this pull request Oct 11, 2022
Merged
@chalin
Copy link
Contributor

chalin commented Oct 11, 2022

@chalin looks like you are Away/OOO. Will merge the PR now, please follow up after you're back, thanks!

Thanks for the original pings and apologies for the delayed response. Yes, a path aliases needed to be added, which is done in:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arminru arminru arminru left review comments

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@reyang reyang reyang approved these changes

@chalin chalin Awaiting requested review from chalin

Assignees

@SergeyKanzhelev SergeyKanzhelev

Labels
editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tigrannajaryan @arminru @carlosalberto @reyang @chalin @bogdandrutu @SergeyKanzhelev