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

Label collection fixes and cleanup #343

Merged
merged 4 commits into from
Feb 17, 2024
Merged

Label collection fixes and cleanup #343

merged 4 commits into from
Feb 17, 2024

Conversation

Nahor
Copy link
Contributor

@Nahor Nahor commented Feb 17, 2024

The most important one is adding the documentation to lib.rs. I didn't know the documentation was there and the README.md was built out of it.

The rest is removing pointless or redundant code.

(The change about chaining the iterators started from a compilation warning about an unnecessarily mutable variable. However
I'm not sure how I was getting that warning, I cannot reproduce it anymore. But I think the change it still worthwhile anyway, but
opinions may vary :))

Because of a typo, the label text was being formatted multiple times per
label in a collection. With the fix, the text is formatted only once per
collection
Since we are going to iterate anyway, instead of growing the label vector,
chain the iterators. This should be more efficient.

In some cases, this also remove a compilation warning about the label
vector being unnecessarily mutable.
- In a label collection, there is no need for a `None` label. One should
  just not add that label
- Code wise it is also incorrect. The wrapper will be for a vector of
  spans, not for individual spans. It happens to work anyway, but this was
  not the intended use.
@zkat zkat merged commit 75fea09 into zkat:main Feb 17, 2024
14 checks passed
@Nahor Nahor deleted the collection2 branch February 18, 2024 01:19
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