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 disable progress bar feature, edit docstrings #3280

Merged
merged 4 commits into from Aug 22, 2019

Conversation

Cubostar
Copy link
Contributor

@Cubostar Cubostar commented Jul 24, 2019

Description

Adds feature explained in #3259. Also edited the docstrings to mention the new progress bool.

@ghost
Copy link

ghost commented Jul 24, 2019

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

@nabobalis nabobalis added this to the 1.0.3 milestone Jul 24, 2019
sunpy/net/helioviewer.py Outdated Show resolved Hide resolved
@nabobalis nabobalis added Feature Request New feature wanted! net Affects the net submodule labels Jul 24, 2019
@pep8speaks
Copy link

pep8speaks commented Jul 24, 2019

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-22 14:59:18 UTC

@Cubostar
Copy link
Contributor Author

Should tests be edited to check if this feature works?

@nabobalis
Copy link
Contributor

I'm not sure I know how you would test it. @Cadair do you know a way?

@Cadair
Copy link
Member

Cadair commented Jul 25, 2019

Easiest way would be to run a download and use pytest to capture the stdout and see if there is a progress bar or not.

(I am like 90% sure you can do that with pytest somehow.)

@Cubostar
Copy link
Contributor Author

I'll do that in ~sunpy.net.tests.test_helioviewer.py then

@Cubostar
Copy link
Contributor Author

Cubostar commented Aug 2, 2019

I haven't been able to test if these new tests I wrote work. Right now I'm assuming that capsys is a fixture (?) from pytest and is not a parameter, but I'm not quite sure how pytest works to run these.

@Cubostar
Copy link
Contributor Author

Cubostar commented Aug 2, 2019

Interestingly, there wasn't an AssertError even though test_progress_png didn't set progress=False.

@Cubostar
Copy link
Contributor Author

Cubostar commented Aug 2, 2019

Don't worry, just a rebase. The duplicate commits are because I reset my developer environment after making this pull request, so the branch progressbar-feat was only tracking the remote branch progressbar-feat on GitHub. The rebase basically made my local branch the official progressbar-feat, hence the extra commits (I think).

@Cubostar
Copy link
Contributor Author

Cubostar commented Aug 7, 2019

Tests now work! 😄

Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

I haven't evaluated the changes at all, but I consider this PR to be non-merge-able until it is properly rebased because the commit history is a mess.

@Cubostar You can ask for help if you don't know to do the rebase

@ayshih
Copy link
Member

ayshih commented Aug 19, 2019

@Cubostar While the general approach to clean up a commit history is through rebasing, you can instead limit yourself to just cherry-picking, which is easier to walk you through. You'd create a fresh branch (here, progressbar-feat-new) and then cherry-pick the commits that you want to keep:

git fetch upstream
git checkout --no-track -b progressbar-feat-new upstream/master
git cherry-pick c007692^..2659b50
git cherry-pick 8ac93ea

After you confirm that the new branch does in fact contain all of your intended changes, you can rename the branch locally – overwriting the old branch – and then force push the branch to your remote:

git branch -M progressbar-feat-new progressbar-feat
git push -f origin progressbar-feat

@Cubostar
Copy link
Contributor Author

WOW everything is so clean! Tysm @ayshih this is amazing

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

Looks good to me barring the removal of the (object) in the class definition.

Co-Authored-By: Nabil Freij <nabil.freij@gmail.com>
@Cubostar
Copy link
Contributor Author

It seems that the CircleCI tests fail if HelioviewerClient doesn't have parentheses. Is this okay with you guys?

@ayshih
Copy link
Member

ayshih commented Aug 22, 2019

The CircleCI builds are currently failing for reasons that are totally unrelated to this PR, not because of the recent change that you made. You don't need to worry about it.

@Cadair Cadair merged commit 13829b0 into sunpy:master Aug 22, 2019
@Cadair
Copy link
Member

Cadair commented Aug 22, 2019

Thanks @Cubostar :)

@Cubostar Cubostar deleted the progressbar-feat branch August 22, 2019 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New feature wanted! net Affects the net submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants