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

Add support to download header information form helioviewer API. #2904

Merged
merged 13 commits into from Jan 30, 2019

Conversation

Projects
None yet
4 participants
@Naman9639
Copy link
Contributor

commented Jan 21, 2019

Description

Fixes #2762

@pep8speaks

This comment has been minimized.

Copy link

commented Jan 21, 2019

Hello @Naman9639! Thanks for updating the PR.

Line 54:101: E501 line too long (101 > 100 characters)
Line 80:101: E501 line too long (117 > 100 characters)
Line 139:101: E501 line too long (106 > 100 characters)
Line 173:101: E501 line too long (121 > 100 characters)
Line 194:101: E501 line too long (116 > 100 characters)
Line 196:101: E501 line too long (102 > 100 characters)
Line 196:103: W291 trailing whitespace
Line 199:101: E501 line too long (107 > 100 characters)
Line 229:101: E501 line too long (121 > 100 characters)
Line 237:101: E501 line too long (107 > 100 characters)
Line 245:17: E203 whitespace before ':'
Line 275:101: E501 line too long (112 > 100 characters)
Line 280:101: E501 line too long (109 > 100 characters)

Line 82:47: E127 continuation line over-indented for visual indent
Line 83:61: E251 unexpected spaces around keyword / parameter equals
Line 83:63: E251 unexpected spaces around keyword / parameter equals

Comment last updated on January 26, 2019 at 12:17 Hours UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Jan 21, 2019

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

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

We require some new modules to be added. That is why the tests are failing. How to solve this?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

Is there no way to do it without the new package?

You will need to add the new package to setup.cfg under the net list. You will need to also add it to the tox.ini file under deps/condadeps (depending on if it's on conda or not).

Also make sure your up to date with master. We had to merge in some fixes for the tests.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

I googled on how to convert xml doc to dictionary so this was the most supported solution

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

I still face problem in rebasing and keeping my branch up to date with master

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

What was the problem?

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

Is there no way to do it without the new package?

You will need to add the new package to setup.cfg under the net list. You will need to also add it to the tox.ini file under deps/condadeps (depending on if it's on conda or not).

Also make sure your up to date with master. We had to merge in some fixes for the tests.

net_requires = drms, zeep, beautifulsoup4, python-dateutil
shall I add the new packages to this list?

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

What was the problem?

My history gets messed up and then I am generally not able to rebase not even merge it with master. I had to clone the updated repository again and that's how it always works for me

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

Yes that is the list.

Make sure you have done:

git remote add upstream git@github.com:sunpy/sunpy.git
git remote update
(on this branch then do)
git rebase -i upstream/master
git push --force

That should work. I would check the log after the rebase.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

Ok I will try that

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

I would try to avoid # in the branch name. It can cause issues.

@nabobalis nabobalis added this to the 1.0 milestone Jan 21, 2019

@nabobalis nabobalis added the net label Jan 21, 2019

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

I would try to avoid # in the branch name. It can cause issues.

Oh ok I will keep that in mind

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

While doing this
git rebase -i upstream/master
Do I need to pick every commit? I always mess up at this step

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

There should be only one commit which is this one which you leave. So you would not edit the file.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

Looks like I already messed up because I am getting a whole lot of commits

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

What are those commits?

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

commits
They are some of the many

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

Well in that case, it would seem somehow your locally git history is incorrect.

I would exit and abort the rebase.

Could you try:
git remote prune upstream
git remote update

That should refresh your local version of upstream hopefully.

@Naman9639 Naman9639 force-pushed the Naman9639:issue#2762 branch from 0159623 to d455138 Jan 21, 2019

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

That worked

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

Do you need to use requests? We used to have it but we removed it. Can you see if you can avoid it?

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

I used it to get the XML doc from the API

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 21, 2019

But we don't seem to need it to access the API for the other functions.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 21, 2019

I can try it using the self. _request function once

Show resolved Hide resolved changelog/2904.feature.rst Outdated
@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 26, 2019

Why is there an error in this?
I am not able to understand this one

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 26, 2019

I think you need to add >>> to the comment lines.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 26, 2019

Can't we add comments without that?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 26, 2019

I do not think so in the examples.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 26, 2019

Oh ok I will keep that in mind

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

old news mate

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 26, 2019

I think just some really minor text changes and it is good to go.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 26, 2019

Text changes? Like some changes in documentation and all that?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 26, 2019

No I just meant the comments I just left. It is just some text changes.

@Naman9639

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 26, 2019

Ok I will do that

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2019

@nabobalis Can you restart those test?

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

These errors are not related to helioviewer right?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

No looks like a test timed out.

@Cadair

Cadair approved these changes Jan 30, 2019

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

Why do it times out again and again?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Some server is just not responding and the test seems to hang. With that in mind, I will merge this now.

@nabobalis nabobalis merged commit 9daaa2e into sunpy:master Jan 30, 2019

9 of 11 checks passed

codecov/patch 18.18% of diff hit (target 75.27%)
Details
sunpy.sunpy Build #20190130.5 has failed
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 Your tests passed on CircleCI!
Details
codecov/project 75.48% (+0.2%) compared to 5db8c6f
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Thanks again @Naman9639

@Naman9639

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

Oh, ok. Thanks

yashrsharma44 added a commit to yashrsharma44/sunpy that referenced this pull request Feb 3, 2019

Add support to download header information form helioviewer API. (sun…
…py#2904)

Add support to download header information form helioviewer API.
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.