Skip to content

fix: safer branch title access#1421

Merged
ianna merged 4 commits intomainfrom
pfackeldey/fix_ak_add_doc
Apr 22, 2025
Merged

fix: safer branch title access#1421
ianna merged 4 commits intomainfrom
pfackeldey/fix_ak_add_doc

Conversation

@pfackeldey
Copy link
Copy Markdown
Collaborator

@pfackeldey pfackeldey commented Apr 18, 2025

This happens when writing a length-0 coffea NanoEvents array with uproot and then reading it again (reported on coffea Mattermost channel). uproot wants to add the branch title as the __doc__ parameter to the awkward array, but fails if it isn't present. This PR looses this requirement a bit and adds an empty string instead of raising a failure.

@pfackeldey pfackeldey requested a review from ianna April 18, 2025 15:05
Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@pfackeldey - Good catch! Thanks. The previous code assumes branch definitely has a title attribute. If not, it will raise an AttributeError.

I think it is better to add something meaningful instead of an empty string, especially for documentation or metadata like __doc__.

Here’s why:

  • An empty string doesn’t help users or tools that rely on metadata to understand or display the data.
  • It can make debugging or documentation harder.
  • It doesn’t signal why the title is missing (was it forgotten, unavailable, not relevant?).

Please, consider an alternative. For example, if it can handle None:

getattr(branch, "title", None)

if not, something like:

getattr(branch, "title", "no title")

Thanks!

@pfackeldey
Copy link
Copy Markdown
Collaborator Author

👍 I changed it to None.

Initially I didn't go for:

getattr(branch, "title", None)

because that's a change of types (it's not guaranteed to be a str anymore).

and I don't like:

getattr(branch, "title", "no title")

because if a branch.title is in fact exactly "no title" there's a double meaning behind this string, which is problematic. That's always a problem of default strings, and I think they should always be avoided if possible.

I was also considering to only add a parameter if the title attribute exists.

In the end I decided to go for None because it's the least invasive of all these changes and likely has the least consequences for downstream libraries or users.

Hope that change does not affect anyone and looks good now 👍

Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@pfackeldey - Thanks!

@ianna ianna merged commit ac7a8c4 into main Apr 22, 2025
26 checks passed
@ianna ianna deleted the pfackeldey/fix_ak_add_doc branch April 22, 2025 15:56
pfackeldey added a commit that referenced this pull request Apr 23, 2025
* safer branch title access

* empty str -> None

---------

Co-authored-by: Ianna Osborne <ianna.osborne@cern.ch>
ianna added a commit that referenced this pull request Oct 28, 2025
* add tree to virtual array conversion

* chore: update pre-commit hooks (#1418)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.4 → v0.11.5](astral-sh/ruff-pre-commit@v0.11.4...v0.11.5)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* ci: pin Chrome version for Pyodide tests (#1422)

* Updated Pyodide version

* Pinned chrome version

* Changed chrome version

* Try using node instead of chrome

* Remove chrome-specific setup

* Actually use Node

* Go back to chrome

* chore: update pre-commit hooks (#1423)

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.5 → v0.11.6](astral-sh/ruff-pre-commit@v0.11.5...v0.11.6)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* fix: issue with empty big_endian array (#1420)

fix issue with empty big_endian array

Co-authored-by: Ianna Osborne <ianna.osborne@cern.ch>

* fix: safer branch title access (#1421)

* safer branch title access

* empty str -> None

---------

Co-authored-by: Ianna Osborne <ianna.osborne@cern.ch>

* docs: add contributing guide (#1425)

* docs: add contributing guide

* style: pre-commit fixes

* Update CONTRIBUTING.md

Co-authored-by: Andres Rios Tascon <ariostas@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: Andres Rios Tascon <ariostas@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: Andres Rios Tascon <ariostas@gmail.com>

* use pre-commit

* build local documentation howto

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Andres Rios Tascon <ariostas@gmail.com>

* improve virtual array loading

* style: pre-commit fixes

* add 'access_log' kwarg

* style: pre-commit fixes

* don't re-define 'Accessed'

* add doc string

* minor doc string fixes

* use form_with_unique_keys from awkward

* rm accidentally committed file...

* refactor virtual buffer loading to better work with form mappings

* require awkward v2.8.2

* merge main

* style: pre-commit fixes

* fix pre-commit

* add virtual kwarg to avoid API sprawl

* satisfy pre-commit

* add special single-branch handling like in .arrays(virtual=False)

* properly forward the recursive function call for single branch cases

* ... also for eager .arrays

* add tests

* pre-commit

* add one test with access_log=None (default)

---------

Co-authored-by: Ianna Osborne <ianna.osborne@cern.ch>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Andres Rios Tascon <ariostas@gmail.com>
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