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

str function for mapbase #4464

Merged
merged 8 commits into from Sep 26, 2020
Merged

str function for mapbase #4464

merged 8 commits into from Sep 26, 2020

Conversation

sidhu1012
Copy link
Contributor

@sidhu1012 sidhu1012 commented Sep 11, 2020

Description

Fixes #4201

@sidhu1012 sidhu1012 requested a review from a team as a code owner September 11, 2020 05:55
@sidhu1012
Copy link
Contributor Author

Why these checks failing on such simple change

@nabobalis nabobalis added this to the 2.1 milestone Sep 11, 2020
@nabobalis nabobalis added the map Affects the map submodule label Sep 11, 2020
@nabobalis
Copy link
Contributor

nabobalis commented Sep 11, 2020

Some of those checks use servers which are prone to failing.

What does the output now look like?

A unit test would be ideal as well.

Can you also change the repr as suggested in the original issue?

@sidhu1012
Copy link
Contributor Author

A unit test would be ideal as well.

Okay

Can you also change the repr as suggested in the original issue?

Sure

@pep8speaks
Copy link

pep8speaks commented Sep 11, 2020

Hello @sidhu1012! 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 2020-09-26 09:08:07 UTC

@nabobalis
Copy link
Contributor

How do the new print and repr look on output?

sunpy/map/mapbase.py Outdated Show resolved Hide resolved
@sidhu1012
Copy link
Contributor Author

Does it seems fine now?

@nabobalis
Copy link
Contributor

How do the new print and repr look on output?

@sidhu1012
Copy link
Contributor Author

How do the new print and repr look on output?

New print :

image

New repr :

image

@nabobalis
Copy link
Contributor

A changelog is required for this change.

@sidhu1012
Copy link
Contributor Author

A changelog is required for this change.

okay

@sidhu1012
Copy link
Contributor Author

A changelog is required for this change.

How to add changelog?

@nabobalis
Copy link
Contributor

A changelog is required for this change.

How to add changelog?

Instructions are here: https://github.com/sunpy/sunpy/blob/master/changelog/README.rst

sidhu1012 and others added 2 commits September 22, 2020 16:19
Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
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.

Thanks for working on this @sidhu1012 it looks good 😄

sunpy/map/tests/test_mapbase.py Show resolved Hide resolved
@sidhu1012
Copy link
Contributor Author

Thanks for working on this @sidhu1012 it looks good 😄

Thanks 😀

Co-authored-by: Stuart Mumford <stuart@cadair.com>
ayshih
ayshih previously requested changes Sep 22, 2020
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.

Still needs a changelog entry (maybe categorized under trivial?)

sunpy/map/tests/test_mapbase.py Show resolved Hide resolved
Co-authored-by: Albert Y. Shih <ayshih@gmail.com>
@sidhu1012
Copy link
Contributor Author

Still needs a changelog entry (maybe categorized under trivial?)

Will add it

@nabobalis
Copy link
Contributor

RTD failure due to issue fixed in master.

@nabobalis nabobalis removed the request for review from ayshih September 26, 2020 11:05
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.

WARNING: :87: (ERROR/3) Unknown interpreted text role "method".

not sure what this is about

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.

fixed on master apparently

@Cadair Cadair merged commit a9a18aa into sunpy:master Sep 26, 2020
@sunpy-backport
Copy link

The backport to 2.0 failed:

Commits ["877250e4a4a38914363ad8af0a6387ed52ea026a","ec61c69ff0fc76d3de635f8f6f5978c7cf8222b3","8b57560225a7fadc0c0b672f21c04727bcae116e","9bdf1f752b8bf03a5dc5076487289a823bf28df6","f534637ca24224abc2e85e04886301bf189eb385","0cd62274bb0bb318a48e13b8f856c14d1e5a02e9","1ca6f1ec6ae68d7676b45480acda6ffc49a58ad0","1a8cbfa413e06862f483f2f255459695b9b3660d"] could not be cherry-picked on top of 2.0

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport 2.0
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 877250e4a4a38914363ad8af0a6387ed52ea026a ec61c69ff0fc76d3de635f8f6f5978c7cf8222b3 8b57560225a7fadc0c0b672f21c04727bcae116e 9bdf1f752b8bf03a5dc5076487289a823bf28df6 f534637ca24224abc2e85e04886301bf189eb385 0cd62274bb0bb318a48e13b8f856c14d1e5a02e9 1ca6f1ec6ae68d7676b45480acda6ffc49a58ad0 1a8cbfa413e06862f483f2f255459695b9b3660d
# Create a new branch with these backported commits.
git checkout -b backport-4464-to-2.0
# Push it to GitHub.
git push --set-upstream origin backport-4464-to-2.0
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is 2.0 and the compare/head branch is backport-4464-to-2.0.

@Cadair
Copy link
Member

Cadair commented Sep 26, 2020

Thanks a lot @sidhu1012 🚀

@nabobalis nabobalis added the Still Needs Manual Backport This PR needs manually backporting label Sep 26, 2020
@dstansby
Copy link
Member

dstansby commented Oct 2, 2020

This changes the functionality of __str__, which has caused some breakages downstream in pfsspy, meaning I'm having to pin sunpy!=2.0.2. In general I think things like this that change behaviour and are not bug-fixes shouldn't be backported, and in general we should be a bit more careful about what we're backporting.

dstansby added a commit to dstansby/sunpy that referenced this pull request Oct 2, 2020
@nabobalis nabobalis removed the Still Needs Manual Backport This PR needs manually backporting label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map Affects the map submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Map __str__
7 participants