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

sunpy.sun python file review #3001

Merged
merged 6 commits into from Mar 30, 2019

Conversation

Projects
None yet
5 participants
@nabobalis
Copy link
Contributor

commented Mar 18, 2019

This time for sun.

TODO:

  • longitude_Sun_perigee, true_latitude and apparent_latitude need to be fixed
  • Add breaking change log.

@nabobalis nabobalis added this to the 1.0 milestone Mar 18, 2019

@pep8speaks

This comment has been minimized.

Copy link

commented Mar 18, 2019

Hello @nabobalis! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 337:101: E501 line too long (184 > 100 characters)
Line 341:101: E501 line too long (160 > 100 characters)

Line 143:101: E501 line too long (184 > 100 characters)

Line 34:101: E501 line too long (126 > 100 characters)
Line 40:101: E501 line too long (190 > 100 characters)
Line 62:101: E501 line too long (126 > 100 characters)

Line 84:101: E501 line too long (151 > 100 characters)

Line 46:101: E501 line too long (124 > 100 characters)
Line 47:101: E501 line too long (124 > 100 characters)
Line 48:101: E501 line too long (124 > 100 characters)

Comment last updated at 2019-03-27 20:48:51 UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Mar 18, 2019

Thanks for the pull request @nabobalis! Everything looks great!

4 similar comments
@sunpy-bot

This comment has been minimized.

Copy link

commented Mar 18, 2019

Thanks for the pull request @nabobalis! Everything looks great!

@sunpy-bot

This comment has been minimized.

Copy link

commented Mar 18, 2019

Thanks for the pull request @nabobalis! Everything looks great!

@sunpy-bot

This comment has been minimized.

Copy link

commented Mar 18, 2019

Thanks for the pull request @nabobalis! Everything looks great!

@sunpy-bot

This comment has been minimized.

Copy link

commented Mar 18, 2019

Thanks for the pull request @nabobalis! Everything looks great!

@nabobalis nabobalis force-pushed the nabobalis:sun branch 3 times, most recently from 0b210fa to f4a8234 Mar 18, 2019

Show resolved Hide resolved sunpy/sun/_constants.py Outdated
@Cadair

Cadair approved these changes Mar 20, 2019

@nabobalis nabobalis force-pushed the nabobalis:sun branch from f4a8234 to f7c3c5f Mar 20, 2019

@nabobalis nabobalis force-pushed the nabobalis:sun branch from f7c3c5f to 15a7a25 Mar 20, 2019

@nabobalis nabobalis force-pushed the nabobalis:sun branch from 15a7a25 to f5136a5 Mar 22, 2019

@nabobalis nabobalis force-pushed the nabobalis:sun branch from 4aae5d1 to cce10e2 Mar 22, 2019

@nabobalis nabobalis force-pushed the nabobalis:sun branch from cce10e2 to b7ba772 Mar 22, 2019

Show resolved Hide resolved setup.cfg Outdated
@Cadair
Copy link
Member

left a comment

This needs the doc line length doing as well.

Show resolved Hide resolved sunpy/sun/sun.py Outdated
@wafels

wafels approved these changes Mar 27, 2019

Copy link
Member

left a comment

I'll update the differential rotation after.

@@ -74,7 +74,7 @@ def position(t='now'):


@add_common_docstring(**_variables_for_parse_time_docstring())
def eccentricity_sunearth_orbit(t='now'):
def eccentricity_sun_earth_orbit(t='now'):

This comment has been minimized.

Copy link
@Cadair

Cadair Mar 29, 2019

Member

The next question becomes should we provide a deprecated wrapper for this?

This comment has been minimized.

Copy link
@nabobalis

nabobalis Mar 29, 2019

Author Contributor

Fair question, I did add the breaking changelog just in case but maybe its not enough

@Cadair

Cadair approved these changes Mar 29, 2019

@nabobalis nabobalis merged commit fe52263 into sunpy:master Mar 30, 2019

14 of 16 checks passed

sunpy.sunpy Build #20190327.12 failed
Details
sunpy.sunpy (Linux_36_Conda_offline)
Details
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip-install Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 86.04%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +13.95% compared to 46cc0a6
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Linux_37_online) Linux_37_online succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details

@nabobalis nabobalis deleted the nabobalis:sun branch Mar 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.