X Tutup
Skip to content

Add warning context#7620

Merged
tacaswell merged 5 commits intomatplotlib:masterfrom
alanbernstein:add-warning-context
Jan 2, 2017
Merged

Add warning context#7620
tacaswell merged 5 commits intomatplotlib:masterfrom
alanbernstein:add-warning-context

Conversation

@alanbernstein
Copy link
Copy Markdown
Contributor

@alanbernstein alanbernstein commented Dec 12, 2016

Thanks for the feedback. Again forgive my ignorance with the workflow here: I reverted the first commit, made a feature branch from that, and created a PR to matplotlib/v2.x.

Your suggestion makes more sense. My first attempt was closer to that, but I was trying to preserve the original warning attributes, and went down the wrong path.

#7601

for path, name in iter_style_files(style_dir):
styles[name] = rc_params_from_file(path, use_default_template=False)
with warnings.catch_warnings(record=True) as warns:
styles[name] = rc_params_from_file(path, use_default_template=False)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line is too long, we enforce pep8 via tooling in the tests.

@tacaswell
Copy link
Copy Markdown
Member

Can you interactively rebase this to squash out the first two commits and then force-push the single commit up to your 2.x branch? git rebase -i

Ping me if you need help with that.

@tacaswell tacaswell dismissed their stale review December 12, 2016 19:04

pep8 addressed.

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Dec 12, 2016
@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Dec 12, 2016

After doing the interactive rebase; do git push --force origin add-warning-context. If you don't force push and follow git's advice to git pull, you will end up with two copies of the commits; one from before you rebased and one from after.

You don't need to do a full rebase to fix this; just reset back to the correct new commits:

$ git branch
# Make sure you are on the 'add-warning-context' branch.

$ git status
# Check you haven't got any leftover changes you want to save with git stash or git commit on another branch.

$ git reset --hard b12ea701712fdb6d2ff9b8943aa6fd9a28b89b38
# This will reset you to the commit you rebased everything into.

$ git push --force origin add-warning-context
# Update the copy on GitHub.

@tacaswell
Copy link
Copy Markdown
Member

👍

Could you add test matplotlib/tests/test_styles.py ?

I suggest using tempdir to make a temporary directory, writing out a know-to-be-bad style file and then using the same warning context manager to check that the warning contains the file name.

Adding this test means that in the future someone won't accidentally break the improvement you have added.

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.0.1 (next bug fix release) Dec 29, 2016
tacaswell
tacaswell previously approved these changes Dec 29, 2016
@NelleV NelleV changed the title Add warning context [MRG+1] Add warning context Dec 30, 2016
@NelleV
Copy link
Copy Markdown
Member

NelleV commented Dec 31, 2016

This patch doesn't work on python3
@alanbernstein do you think you could fix the issue?

Else, it looks good.

@tacaswell tacaswell dismissed their stale review December 31, 2016 16:37

Because the test does not work on python3

@tacaswell tacaswell changed the base branch from v2.x to master January 2, 2017 04:10
@tacaswell tacaswell closed this Jan 2, 2017
@tacaswell tacaswell reopened this Jan 2, 2017
@tacaswell
Copy link
Copy Markdown
Member

I re-targeted this PR at the master branch and will merge it once it passes will merge it and leave it tagged as 2.0.1.

This should be backported after we have 2.0 final out.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 2, 2017

Current coverage is 62.12% (diff: 100%)

Merging #7620 into master will increase coverage by <.01%

@@             master      #7620   diff @@
==========================================
  Files           174        174          
  Lines         56023      56027     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34800      34804     +4   
  Misses        21223      21223          
  Partials          0          0          

Powered by Codecov. Last update db685c7...69e1092

@tacaswell tacaswell merged commit 8279f64 into matplotlib:master Jan 2, 2017
@Kojoley Kojoley changed the title [MRG+1] Add warning context Add warning context Jan 11, 2017
@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Jan 30, 2017

Backported to v2.0.x as 4846dea.

QuLogic pushed a commit that referenced this pull request Jan 30, 2017
ENH: Add warning context to style files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

X Tutup