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

Ensures that Helioviewer client uses Helioviewer API version 2 exclusively #2801

Merged
merged 34 commits into from Dec 31, 2018

Conversation

Projects
None yet
7 participants
@Naman9639
Copy link
Contributor

commented Oct 18, 2018

Description

Fixes #2280

@pep8speaks

This comment has been minimized.

Copy link

commented Oct 18, 2018

Hello @Naman9639! Thanks for updating the PR.

Line 21:1: E302 expected 2 blank lines, found 1
Line 41:101: E501 line too long (191 > 100 characters)
Line 43:101: E501 line too long (104 > 100 characters)
Line 45:101: E501 line too long (101 > 100 characters)
Line 46:1: W293 blank line contains whitespace
Line 71:101: E501 line too long (119 > 100 characters)
Line 160:101: E501 line too long (164 > 100 characters)
Line 174:23: E203 whitespace before ':'
Line 188:86: W291 trailing whitespace
Line 272:101: E501 line too long (159 > 100 characters)
Line 273:101: E501 line too long (193 > 100 characters)
Line 275:1: W293 blank line contains whitespace

Line 77:1: W293 blank line contains whitespace
Line 77:1: W391 blank line at end of file

Comment last updated on December 23, 2018 at 07:32 Hours UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Oct 18, 2018

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

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 18, 2018

Why are all these extra commits are there in this new branch I created

@nabobalis nabobalis added this to the 0.9.4 milestone Oct 18, 2018

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 18, 2018

Thats a good question.

I think the easiest method to fix this would be to add this repository as a remote (normally called upstream) then

git remote add upstream git://github.com/sunpy/sunpy.git (or the https version)
git fetch upstream
git rebase -i upstream/master (remove the commits with the discard option)
git push --force
@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 18, 2018

Oh ok I will try this

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 18, 2018

Only git push --force when you have checked the history with git log

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 18, 2018

https://api.helioviewer.org/v2/
This URL is not working

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 18, 2018

Is that the correct URL from the API documentation?

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 18, 2018

That is the URL mentioned in the issue

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 18, 2018

Doesn't mean its correct.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 18, 2018

Yeah, I guess the word docs is missing?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 18, 2018

I'm not sure. You'd have to read the API documentation to check. It's hopefully listed there. I think it's linked in the original issue.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 18, 2018

Ok thank you

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 18, 2018

Will you restart this test please ci/circleci: figure-tests-36

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 18, 2018

Done.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 18, 2018

Thanks

Show resolved Hide resolved sunpy/net/helioviewer.py Outdated

@nabobalis nabobalis modified the milestones: 0.9.4, 1.0 Oct 19, 2018

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 19, 2018

Do I need to change the test files as well?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 19, 2018

You mean the tests? If the change in API means the tests now fail, they need to be changed too.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 19, 2018

But we test it with and without sourceId and now in most of the function sourceId is compulsory.

I will figure it out

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 19, 2018

Well then that test should be deleted or changed to test something else.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 19, 2018

Ok I will see what I can do

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 20, 2018

I guess I need to start a new PR
My laptop crashed and I lost all the PRs and changes I made so I had to set up everything again
Is there a way out?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Oct 20, 2018

As long as they exist on GitHub, you can just re-clone the repo and carry on.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Oct 20, 2018

Nope they are not on my Git

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Dec 22, 2018

Ok sure I will do that

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Dec 22, 2018

There are still errors

From https://github.com/Naman9639/sunpy
 * branch            master     -> FETCH_HEAD
Auto-merging sunpy/net/tests/test_helioviewer.py
CONFLICT (content): Merge conflict in sunpy/net/tests/test_helioviewer.py
Auto-merging sunpy/net/helioviewer.py
CONFLICT (content): Merge conflict in sunpy/net/helioviewer.py
Auto-merging docs/guide/acquiring_data/helioviewer.rst
CONFLICT (content): Merge conflict in docs/guide/acquiring_data/helioviewer.rst
Automatic merge failed; fix conflicts and then commit the result.
@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Dec 22, 2018

To be expected, using git mergetool should allow you to fix those.

@Naman9639 Naman9639 force-pushed the Naman9639:issue#2280 branch from 4896e65 to 3a8a82b Dec 22, 2018

Show resolved Hide resolved changelog/2801.bugfix.rst Outdated

@Naman9639 Naman9639 force-pushed the Naman9639:issue#2280 branch from c38d446 to a768f01 Dec 22, 2018

Show resolved Hide resolved changelog/2801.breaking.rst Outdated
Show resolved Hide resolved docs/guide/acquiring_data/helioviewer.rst Outdated
Show resolved Hide resolved sunpy/net/tests/test_helioviewer.py Outdated

@Naman9639 Naman9639 force-pushed the Naman9639:issue#2280 branch from 84edb77 to 2a01dc2 Dec 23, 2018

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Dec 23, 2018

Are these changes ok?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Dec 23, 2018

Yeah, they look good to me.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Dec 24, 2018

Awesome. Let's wait for others

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Um, is everyone still busy?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2018

It is still the Christmas/New Year break period. Reviews will come when they come, there is no reason to rush it.

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Yeah, I can understand. Actually I was thinking for GSOC so that is why I was a bit hyper. Sorry

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2018

GSoC is still awhile away and the rule (if i recall correctly) is one open pull request for GSoC consideration (plus the application and all that).

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Ok, I will wait.

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2018

Yeah the program doesn't open till next year and it's not first come first serve. So there is no urgency for now.

I'm sure this will be merged in due time.

@Cadair

Cadair approved these changes Dec 30, 2018

@nabobalis nabobalis merged commit 9cfeb5d into sunpy:master Dec 31, 2018

11 checks passed

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
codecov/patch 96.29% of diff hit (target 75.2%)
Details
codecov/project 86.41% (+11.2%) compared to 3bdb581
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20181223.6 succeeded
Details
@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2018

Thank you @Naman9639 for this!

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2019

Thanks and sorry for the rush

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.