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 *CoordGenerator classes #2558

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Conversation

eerovaher
Copy link
Member

astroquery.utils.commons defined a few classes which allowed constructing astropy.coordinates.SkyCoord instances without having to specify the frame keyword at the cost of having to specify the correct class instead. They provided little value, and all the code that used them now uses SkyCoord directly.

Contributes towards #2096 and #2429.

@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #2558 (c029146) into main (5af5780) will decrease coverage by 0.01%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##             main    #2558      +/-   ##
==========================================
- Coverage   63.97%   63.95%   -0.02%     
==========================================
  Files         132      132              
  Lines       16984    16975       -9     
==========================================
- Hits        10866    10857       -9     
  Misses       6118     6118              
Impacted Files Coverage Δ
astroquery/ipac/irsa/irsa_dust/core.py 81.62% <83.33%> (+0.04%) ⬆️
astroquery/utils/commons.py 77.24% <88.88%> (-1.15%) ⬇️

📣 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.

The notion of this PR is great and highly appreciated, but please don't reformat the lines and add practically empty ones. We allow longer lines, many of the changes fit into a one lines, please don't cut them off at 80/88 and make it a 3 liner.

response = heasarc.query_region_async(c, mission=mission,
radius='1 degree')
response = heasarc.query_region_async(
skycoord_3C_273, mission=mission, radius="1 degree"
Copy link
Member

Choose a reason for hiding this comment

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

These should very much fit into one line.

radius='1 degree')
response = heasarc.query_region_async(
skycoord_3C_273, mission=mission, radius="1 degree"
)
Copy link
Member

Choose a reason for hiding this comment

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

we don't do black here, please don't have hanging closing parens if possible

dec=69.065294722 * u.deg,
frame='icrs')),
'regSize': 5}), ])
(
Copy link
Member

Choose a reason for hiding this comment

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

again, don't use black, please reformat without empty lines with single parens and trailing comas

OBJ_LIST = [
"m31",
"00h42m44.330s +41d16m07.50s",
SkyCoord(l=121.1743 * u.deg, b=-21.5733 * u.deg, frame="galactic")
Copy link
Member

Choose a reason for hiding this comment

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

keep only the PR relevant changes, e.g. getting rid of the coorgenerator, please don't reformat

Comment on lines 124 to 128
poly1 = [
SkyCoord(ra=10.1 * u.deg, dec=10.1 * u.deg),
SkyCoord(ra=10.0 * u.deg, dec=10.1 * u.deg),
SkyCoord(ra=10.0 * u.deg, dec=10.0 * u.deg),
]
Copy link
Member

Choose a reason for hiding this comment

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

Keep the style, the additional 2 lines don't add anyting

Suggested change
poly1 = [
SkyCoord(ra=10.1 * u.deg, dec=10.1 * u.deg),
SkyCoord(ra=10.0 * u.deg, dec=10.1 * u.deg),
SkyCoord(ra=10.0 * u.deg, dec=10.0 * u.deg),
]
poly1 = [SkyCoord(ra=10.1 * u.deg, dec=10.1 * u.deg),
SkyCoord(ra=10.0 * u.deg, dec=10.1 * u.deg),
SkyCoord(ra=10.0 * u.deg, dec=10.0 * u.deg)]

unit=(u.deg, u.deg)),
frame_type='interleave', programme_id="GCS", waveband="K",
radius=20 * u.arcmin)
icrs_skycoord,
Copy link
Member

Choose a reason for hiding this comment

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

these can now very much fit into one or max two lines

@bsipocz bsipocz added this to the v0.4.7 milestone Oct 18, 2022
`astroquery.utils.commons` defined a few classes which allowed
constructing `astropy.coordinates.SkyCoord` instances without having to
specify the `frame` keyword at the cost of having to specify the correct
class instead. They provided little value, and all the code that used
them now uses `SkyCoord` directly.
@eerovaher
Copy link
Member Author

I've reformatted the lines I've edited so that there are no longer any brackets between whitespace characters, or between a whitespace character and a trailing comma.

@bsipocz bsipocz merged commit 9db9386 into astropy:main Oct 20, 2022
@bsipocz
Copy link
Member

bsipocz commented Oct 20, 2022

Thanks @eerovaher!

@eerovaher eerovaher deleted the rm-coordgenerators branch October 20, 2022 15:19
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