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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for a default aliases file #398

Merged
merged 3 commits into from May 5, 2023
Merged

Conversation

JamesWrigley
Copy link
Member

Following up on #367 (comment), I've added support for an aliases argument to open_run() that defaults to looking for a file named usr/extra-data-aliases.yml in the proposal. So as long as that file exists, you get aliases by default. Let the bikeshedding begin! 馃榾

Things to mention:

  • I figured that it only made sense to add an aliases argument to open_run() and not RunDirectory because this automatic-aliases-finding thing is specifically for opening runs by proposal. By that argument you could say that the aliases argument should only support paths and not dict objects, but I figured it could be useful. I have no objection to removing support for dict arguments though.
  • One of .with_aliases() possible inputs is an *arg of tuples, but I didn't think that API made sense for open_run() so I left it out.

b29ad11 and b10e82c are smaller fixes that I found useful.

extra_data/reader.py Fixed Show fixed Hide fixed
Copy link
Contributor

@philsmt philsmt left a comment

Choose a reason for hiding this comment

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

Thanks!

Only a few nitpicks implementation-wise. While thinking about the potential silentless of this feature, I was wondering if we should just list all aliases in DataCollection.info(), rather than attach it to sources/keys if they happen to be shown (by default not the case for keys).

extra_data/reader.py Outdated Show resolved Hide resolved
extra_data/reader.py Outdated Show resolved Hide resolved
extra_data/reader.py Outdated Show resolved Hide resolved
@JamesWrigley
Copy link
Member Author

While thinking about the potential silentless of this feature, I was wondering if we should just list all aliases in DataCollection.info(), rather than attach it to sources/keys if they happen to be shown (by default not the case for keys).

What if it logged something when an alias is loaded? I'm mildly against listing all the aliases because .info() can be pretty long already.

@JamesWrigley
Copy link
Member Author

Ugh, so beautiful 鉂わ笍

image

@philsmt
Copy link
Contributor

philsmt commented Apr 26, 2023

While thinking about the potential silentless of this feature, I was wondering if we should just list all aliases in DataCollection.info(), rather than attach it to sources/keys if they happen to be shown (by default not the case for keys).

What if it logged something when an alias is loaded? I'm mildly against listing all the aliases because .info() can be pretty long already.

That is correct. I was thinking of warnings/logs too, but wasn't sure people will actually see it. Do we mind just printing something in open_run?

Useful for checking if some alias exists, e.g. with `"foo" in run.alias`.
@JamesWrigley
Copy link
Member Author

Added a print() statement in 871512b, and squashed all the fixup commits.

@philsmt
Copy link
Contributor

philsmt commented May 5, 2023

Neat, LGTM

@JamesWrigley JamesWrigley merged commit cfca21f into master May 5, 2023
8 of 9 checks passed
@JamesWrigley JamesWrigley deleted the aliases_location branch May 5, 2023 08:14
@takluyver takluyver added this to the 1.13 milestone May 8, 2023
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.

None yet

3 participants