Skip to content

ENH: astropy.io.votable.exceptions.conf.max_warnings#10152

Merged
pllim merged 1 commit intoastropy:masterfrom
pllim:vo-max-warn-cfg
Apr 21, 2020
Merged

ENH: astropy.io.votable.exceptions.conf.max_warnings#10152
pllim merged 1 commit intoastropy:masterfrom
pllim:vo-max-warn-cfg

Conversation

@pllim
Copy link
Member

@pllim pllim commented Apr 15, 2020

Description

This pull request is to add a new configuration item to replace hardcoded MAX_WARNINGS in astropy.io.votable.exceptions.

Fixes #7529

@pllim pllim added this to the v4.1 milestone Apr 15, 2020
@pllim pllim requested a review from tomdonaldson April 15, 2020 20:33
Copy link
Contributor

@tomdonaldson tomdonaldson left a comment

Choose a reason for hiding this comment

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

This looks good to me. Just had one minor question for my own education.


__all__ = [
'warn_or_raise', 'vo_raise', 'vo_reraise', 'vo_warn',
'Conf', 'conf', 'warn_or_raise', 'vo_raise', 'vo_reraise', 'vo_warn',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the capital C Conf needed here? I didn't notice it being referenced, but I'm probably missing some of the black magic needed for setup.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copy-pasted that part from the __init__.py file. If you feel strongly about it, I can remove it. I think all this affects is the from astropy.io.votable.exceptions import * command and also when Sphinx scrapes the files for API doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, actually, I do need to change the doc a little to make it render.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed the doc update. Given that the doc doesn't use automodapi for exceptions, this addition only affects star-imports. Are you okay with leaving this in or you want me to take it back out? Please advise, @tomdonaldson . Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine leaving it in since I'm not sure enough that it would never be useful. It's certainly not harmful in any important way.

@pllim pllim force-pushed the vo-max-warn-cfg branch 2 times, most recently from bf22a6b to ec32690 Compare April 16, 2020 20:46
@pllim pllim force-pushed the vo-max-warn-cfg branch from ec32690 to 79c4a4d Compare April 20, 2020 22:05
@pllim pllim force-pushed the vo-max-warn-cfg branch from 28dff5c to 30594b0 Compare April 21, 2020 03:05
@pllim pllim added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Apr 21, 2020
@pllim pllim merged commit f58cf9f into astropy:master Apr 21, 2020
@pllim pllim deleted the vo-max-warn-cfg branch April 21, 2020 03:49
@pllim
Copy link
Member Author

pllim commented Apr 21, 2020

Thanks for the review, @tomdonaldson !

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

Labels

Enhancement io.votable zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make io.votable.exceptions.MAX_WARNINGS configurable

2 participants