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

:zoom-in with too large count doesn't change zoom #1118 #1140

Closed
wants to merge 17 commits into from

Conversation

@shawnaxsom
Copy link

@shawnaxsom shawnaxsom commented Nov 25, 2015

This is related to issue #1118. Typing 999999999+ should result in the max standard zoom, 500%. Typing 999999999- should result in min standard zoom, 25%. Minor refactorings, mainly making the __snap_in method able to run whether current zoom is fuzzy or not for consistency.

Previous request was closed after I found a few issues that needed to be resolved.


This change is Reviewable

@codecov-io
Copy link

@codecov-io codecov-io commented Nov 25, 2015

Current coverage is 68.77%

Merging #1140 into master will decrease coverage by -0.27% as of a7736f8

@@            master   #1140   diff @@
======================================
  Files          102     102       
  Stmts        13889   13907    +18
  Branches      2191    2195     +4
  Methods          0       0       
======================================
- Hit           9589    9564    -25
- Partial        419     428     +9
- Missed        3881    3915    +34

Review entire Coverage Diff as of a7736f8

Powered by Codecov. Updated on successful CI builds.

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Nov 26, 2015

Thanks for fixing the tests/linters!

It seems you included coverage.xml again though - it should be deleted automatically after the tests (by check_coverage.py), and you can safely delete it (and remove it from your PR please).

I'll add it to .gitignore.

@shawnaxsom
Copy link
Author

@shawnaxsom shawnaxsom commented Nov 26, 2015

Ah sorry! It is removed from the PR now, thank you for your patience. And I
am glad to help with tests/linters, I wouldn't want to check anything in
that would lower coverage or cause headaches for anyone else.

Shawn Axsom

On Thu, Nov 26, 2015 at 1:01 PM, Florian Bruhin notifications@github.com
wrote:

Thanks for fixing the tests/linters!

It seems you included coverage.xml again though - it should be deleted
automatically after the tests (by check_coverage.py), and you can safely
delete it (and remove it from your PR please).

I'll add it to .gitignore.


Reply to this email directly or view it on GitHub
#1140 (comment)
.

@shawnaxsom
Copy link
Author

@shawnaxsom shawnaxsom commented Nov 26, 2015

@The-Compiler Know the issue here? I need to run for a few hours for Thanksgiving, I can take a look after that. I only removed the converage.xml, I'm not sure if appveyor was looking for that and can't find it now.

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Nov 26, 2015

No idea what's going on - I'll just run the tests locally once I get to review this. Have fun!

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Nov 27, 2015

I think Appveyor and Travis somehow have given up because there are merge conflicts (as I removed the trailing . required for the "... should be shown" test).

I'll add some line comments.

@@ -149,6 +171,13 @@ def _get_new_item(self, offset):
new = self.curitem()
elif self._mode == self.Modes.exception: # pragma: no branch
raise
elif self._mode == self.Modes.edge:

This comment has been minimized.

@The-Compiler

The-Compiler Nov 27, 2015
Member

Can you move the # pragma: no branch line from above to here? This is to tell coverage.py to not mark the (not existing) "else:" branch of this if as not taken, as there's no way this could happen.

This comment has been minimized.

@shawnaxsom

shawnaxsom Nov 27, 2015
Author

Done :)

items = None

is_int = lambda s: isinstance(self.items[self._idx], int) \
or s.lstrip("-+").isdigit()

This comment has been minimized.

@The-Compiler

The-Compiler Nov 27, 2015
Member

I don't understand why you added this check - I see some test_cmdhistory.py tests are failing if I remove it, but I don't get why (though it's been a while since I last looked at NeighborList code).

This comment has been minimized.

@shawnaxsom

shawnaxsom Nov 27, 2015
Author

Line 121 checks integers, line 122 checks for strings. test_cmdhistory.py has some cases that use strings in the neighborlist, like test_nextitem_previtem_chain I believe. It is like: ['first', 'second', 'third', 'fourth', 'fifth']. In that case it worked fine for it to instead go into line 138 and have it sort the list; it can't go into line 136 and do some subtraction math.

Shawn Axsom added 3 commits Nov 27, 2015
…he (not existing) 'lse:' branch of this if as not taken, as there's no way this could happen.
@shawnaxsom
Copy link
Author

@shawnaxsom shawnaxsom commented Nov 28, 2015

It looks like this PR is ready to merge if you're good with it. Let me know if you need anything else. Thanks!

The-Compiler added a commit that referenced this pull request Jun 6, 2016
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jun 6, 2016

Sorry for the looooong delay on this one!

When I first reviewed it, I didn't really understand it, so I kind of forgot about it. I now took another look, and I believe I found a simpler way with the same behaviour: abfd789

I also got rid of the block and wrap modes as they're not used anywhere anyways.

Thanks for the contribution though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants