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

Use : instead of . as the entity:component separator in paths #4471

Merged
merged 3 commits into from Dec 12, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Dec 11, 2023

What

This is a very minor breaking change, where existing links in markdown to specific components of an entity will break.

Before: recording://helix/structure.Transform3D
After: recording://helix/structure:Transform3D

The purpose is to allow . as a valid character in a path part, e.g. as images/foo.jpg

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
    • Full build: app.rerun.io
    • Partial build: app.rerun.io - Useful for quick testing when changes do not affect examples in any way
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@emilk emilk marked this pull request as ready for review December 11, 2023 16:25
@nikolausWest
Copy link
Member

Are there any "custom data" examples that should be updated?

Copy link
Contributor

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

👍🏻

@emilk
Copy link
Member Author

emilk commented Dec 11, 2023

Are there any "custom data" examples that should be updated?

I don't know if any of them use direct links to components.
I suggest we take a pass on updating them all once #4464 is completed and published in 0.13.

Let's wait with merging this until after the coming 0.12 release, to gather all entity-path related changes up in one PR.

@emilk emilk added the do-not-merge Do not merge this PR label Dec 11, 2023
@teh-cmc
Copy link
Member

teh-cmc commented Dec 12, 2023

I ended up implementing a very hackish version of this on my file-loading branch, because otherwise rerun my_folder_full_of_images would result in a wall of warnings.

I think we should merge this ASAP if we're going to ship the file-loading branch with 0.12.

@teh-cmc teh-cmc added 🔩 data model 😤 annoying Something in the UI / SDK is annoying to use and removed do-not-merge Do not merge this PR labels Dec 12, 2023
@teh-cmc
Copy link
Member

teh-cmc commented Dec 12, 2023

We decided to go through with this

@teh-cmc teh-cmc merged commit 7358cb1 into main Dec 12, 2023
43 of 52 checks passed
@teh-cmc teh-cmc deleted the emilk/better-entity-paths branch December 12, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😤 annoying Something in the UI / SDK is annoying to use 🔩 data model include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants