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

Remove needless argument checks from gaia #2682

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

eerovaher
Copy link
Member

astroquery.gaia.GaiaClass contained a handful of needless argument checks:

  • In load_data() separately checking for retrieval_type being None (its default value is "ALL") is not needed because the value is checked later on, unless avoid_datatype_check is explicitly set to True:
    if avoid_datatype_check is False:
    # we need to check params
    rt = str(retrieval_type).upper()
    if rt != 'ALL' and rt not in self.VALID_DATALINK_RETRIEVAL_TYPES:
    raise ValueError(f"Invalid mandatory argument 'retrieval_type'. Found {retrieval_type}, "
    f"expected: 'ALL' or any of {self.VALID_DATALINK_RETRIEVAL_TYPES}")
  • In load_data() checking for ids being None only makes a difference if None is the explicitly specified value, but any other possible invalid value would pass this check, so it is not very useful.
  • In cross_match() several arguments were given default values and the code then checked if any of those arguments had the default value. It is better to not specify the default value at all and to let Python itself ensure that the mandatory argument values are specified. This is not only simpler but also ensures that the error message contains the correct argument name, and if multiple arguments are missing then Python's error message makes that clear too.

`astroquery.gaia.GaiaClass` contained a handful of needless argument
checks:

- In `load_data()` separately checking for `retrieval_type` being `None`
  is not needed because the value is checked later on, unless
  `avoid_datatype_check` is explicitly set to `True`.
- In `load_data()` checking for `ids` being `None` only makes a
  difference if `None` is the explicitly specified value, but any other
  possible invalid value would pass this check, so it is not very useful.
- In `cross_match()` several arguments were given default values and the
  code then checked if any of those arguments had the default value. It
  is better to not specify the default value at all and to let Python
  itself ensure that the mandatory argument values are specified.
@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #2682 (df3b6fc) into main (03c8420) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2682      +/-   ##
==========================================
- Coverage   69.19%   69.18%   -0.01%     
==========================================
  Files         304      304              
  Lines       22539    22529      -10     
==========================================
- Hits        15595    15587       -8     
+ Misses       6944     6942       -2     
Impacted Files Coverage Δ
astroquery/gaia/core.py 70.77% <100.00%> (-0.32%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Thanks @eerovaher, these all look great!

def cross_match(self, *, full_qualified_table_name_a=None,
full_qualified_table_name_b=None,
results_table_name=None,
def cross_match(self, *, full_qualified_table_name_a,
Copy link
Member

Choose a reason for hiding this comment

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

yes, I suppose we could have kept these positional args, but I think there is no need to change them now.

@bsipocz bsipocz added this to the v0.4.7 milestone Mar 13, 2023
@bsipocz bsipocz merged commit a4716fd into astropy:main Mar 13, 2023
@eerovaher eerovaher deleted the gaia-mandatory-kwarg-checks branch March 13, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants