Skip to content

Conversation

@kotfu
Copy link
Member

@kotfu kotfu commented Mar 10, 2019

Improve history and associated documentation as discussed in #640 and #641.

Closes #640
Closes #641

@codecov
Copy link

codecov bot commented Mar 10, 2019

Codecov Report

Merging #645 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #645      +/-   ##
=========================================
- Coverage   94.31%   94.3%   -0.01%     
=========================================
  Files          11      11              
  Lines        3058    3057       -1     
=========================================
- Hits         2884    2883       -1     
  Misses        174     174
Impacted Files Coverage Δ
cmd2/cmd2.py 94.38% <100%> (+0.02%) ⬆️
cmd2/history.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e365db3...4e91cc6. Read the comment docs.

Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

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

The following should be fixed:

  • flake8 error due to assigned but unused variable
  • incorrectly treating a span-end negative index as exclusive

We should research and see if there is a way around the weird argparse behavior where it is treating a positional argument starting with a "-" as a flag.

tleonhardt
tleonhardt previously approved these changes Mar 11, 2019
Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

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

Recommend fixing the one flake8 error I found on my system. Other than that, nice work - particularly in the documentation arena.

@tleonhardt
Copy link
Member

Make surge a single ending negative index in a span works correctly. Other than that looks good I think.

@kotfu
Copy link
Member Author

kotfu commented Mar 11, 2019

@tleonhardt, as I understand your feedback, an ending negative index should include the referenced item. If I had four items in the history, and typed history 1:-1 you would expect all four items to be returned.

Here was my thinking when I implemented the current behavior. Python's native zero-based indices have 0 as the first element of the array, and -1 as the last element of the array, like so:

 0  1  2  3  4  5  6  7  8 
 S  O  M  E  W  O  R  D  S
-9 -8 -7 -6 -5 -4 -3 -2 -1

If we shift the positive indices to the left to make them 1 based, should we not also shift the negative indices to the left on my lame ASCII chart as well? If we don't, then the size of the slice returned by history 1:-1 changes. With native zero based indices (on a list with 3 or more elements) list[1:-1] returns a list two shorter than the original list. If I implement your feedback, history 1:-1 returns a list only one shorter than the original list.

I am open to either solution, but just wanted to explain my thinking. Lemme know if you still think I should make the requested modification, and I'll be happy to do it.

@tleonhardt
Copy link
Member

@kotfu
We are in really unfamiliar territory here since with the history command, we are doing something significantly different than either Python or Bash history.

In Python, list indexing is 0-based and list slicing is inclusive of the start index and exclusive of the end index. In Bash, the history command there is no slicing or range extraction - history by itself returns the entire history and history 5 returns the last 5 elements of the history.

In the cmd2 history command, history 5 returns the 5th element of the history and history 1:3 returns the first three history elements - so it is 1-based indexing and slicing is inclusive of both start and end index. If we say that history -1 returns the last element in the history, then I would think that slicing should still be inclusive of both indices. Adjusting negative indices to be "1-based" would result in the crazy situation that index 0 would refer to the last element - which I'm sure none of us want ;-)

I'm pretty flexible to how we do things, I just want to make sure we are internally logically consistent and well documented. Three possibilities I see include:

  1. Keep things mostly the way they are now and just make slicing always inclusive of both the start and end index, even when the end index is negative
    • This is the least work and most backward compatible solution
  2. Move to an entirely Pythonic list indexing and slicing syntax where it is 0-based and exclusive of the end index
    • This breaks backwards compatibility, but would potentially be good if we expect end users of cmd2 applications to be familiar with Python
  3. Keep things mostly the way they are now, but make slicing always exclusive of the end index like Python.
    • This mildly breaks backward compatibility, but honestly I don't see a real advantage here

What do you folks think?

@kotfu
Copy link
Member Author

kotfu commented Mar 12, 2019

After thinking about it for a day, I think I like option #1. It's much easier to explain to a regular person that "1 is the first command, -1 is the last command. 2 is the second command, -2 is the second to last command." Having it be inclusive make it nice and easy to explain, and I'm a big fan of clarity.

I'll make the change and get it pushed so you can review.

@tleonhardt
Copy link
Member

I did some manual testing and am now very happy with the way things work in this PR. Nice job!

@tleonhardt tleonhardt merged commit a127997 into master Mar 12, 2019
@tleonhardt tleonhardt deleted the history_improvements branch March 12, 2019 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants