Skip to content

Raise instead of silently ignore when val2 is provided but will be ignored#9373

Merged
bsipocz merged 3 commits intoastropy:masterfrom
aarchiba:reject-ignored-val2
Oct 17, 2019
Merged

Raise instead of silently ignore when val2 is provided but will be ignored#9373
bsipocz merged 3 commits intoastropy:masterfrom
aarchiba:reject-ignored-val2

Conversation

@aarchiba
Copy link
Contributor

@aarchiba aarchiba commented Oct 15, 2019

Currently various Time functions that don't use val2 will silently ignore the value provided by the user. With this PR a ValueError is raised instead.

@astropy-bot astropy-bot bot added the time label Oct 15, 2019
@aarchiba aarchiba mentioned this pull request Oct 15, 2019
Merged
@mhvk
Copy link
Contributor

mhvk commented Oct 16, 2019

I'm happy with the change, but since this is an API change, I'll ping @taldcroft too. My own sense is that we do not have to go through a deprecation for this, but it is not difficult to do.

If we do go ahead, it will need an entry under API changes in CHANGES.rst

@mhvk mhvk added this to the v4.0 milestone Oct 16, 2019
@mhvk mhvk added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Oct 16, 2019
@mhvk mhvk requested a review from taldcroft October 16, 2019 13:00
@aarchiba
Copy link
Contributor Author

aarchiba commented Oct 16, 2019

I'm happy with the change, but since this is an API change, I'll ping @taldcroft too. My own sense is that we do not have to go through a deprecation for this, but it is not difficult to do.

If we do go ahead, it will need an entry under API changes in CHANGES.rst

I've no problem with writing that, and I can toss in a few more tests to appease the coverage machine. Just let me know if it is wanted.

mhvk
mhvk approved these changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Approved by me, but I'd still like @taldcroft's input; @taldcroft - any chance for a quick look?

taldcroft
taldcroft approved these changes Oct 17, 2019
View reviewed changes
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bsipocz bsipocz merged commit 8b9588d into astropy:master Oct 17, 2019
@bsipocz
Copy link
Member

bsipocz commented Oct 17, 2019

Thank you @aarchiba!

@aarchiba aarchiba deleted the reject-ignored-val2 branch October 18, 2019 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taldcroft taldcroft taldcroft approved these changes

@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period time

Projects

None yet

Milestone

v4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@aarchiba @mhvk @bsipocz @taldcroft