Skip to content

Conversation

@frcroth
Copy link
Contributor

@frcroth frcroth commented Sep 18, 2023

Supports listing keys and getting keys. There is a delete button, which I did not implement because I did not want to destroy my annotations (but should be easily done).

Edit: CI doesn't work because of ancient python version

@frcroth frcroth requested a review from fm3 September 18, 2023 08:02
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

This is a great new tool in our toolbox! Making me happy!

It already works quite well. Would it be hard to add the following features from my wish-list?

  • Add a button similar to k to show the previous keys? Right now, I can only navigate in one direction
  • Instead of these two buttons, have the arrow keys up/down move to the next keys when the end/start of the current list is reached?
  • Make the fossildb address configurable (but default to localhost:7155)? This could also be a command line argument
  • Have the output files be named after the key and the version? e.g. <key>_v<version> rather than out.bin
  • Remove the delete button for now (I think delete is not a common use case, and it does not work now anyway)

For the moment, let’s assess the effort required for these features, and not start right away with implementing. We can then discuss the priority.

@frcroth
Copy link
Contributor Author

frcroth commented Oct 9, 2023

  • Add a button similar to k to show the previous keys? Right now, I can only navigate in one direction

With a list of previous offsets that should be relatively easy.

  • Instead of these two buttons, have the arrow keys up/down move to the next keys when the end/start of the current list is reached?

Would need to take a look at how the list works. Most cli applications have 2 ways of navigating, page up/down and line by line. The arrow down (or j and k in vi) should only scroll one line.

  • Make the fossildb address configurable (but default to localhost:7155)? This could also be a command line argument

Via file trivial, don't know immediately know how to do it via cli because we don't really have control of the startup procedure. But should be a common use case and not too hard.

  • Have the output files be named after the key and the version? e.g. _v rather than out.bin

Seems very straight forward. Not clear what naming scheme you're proposing: <key_name>_v ?

  • Remove the delete button for now (I think delete is not a common use case, and it does not work now anyway)

Well that's easy.

@fm3
Copy link
Member

fm3 commented Oct 10, 2023

Thanks for your thoughts! I think we should go for these ideas. On output file naming: sorry, markup ate my text. I meant <key>_v<version>

I think having a list of previous offsets to support scrolling back is fair :) Might lead to slightly wrong offsets if the server content has changed, but I think that should not do much harm.

On command line arguments: I would guess that you could do argparse before the app = FossilDBClient(stub) line, and pass the result as a second parameter there.

If changing the scrolling behavior is hard, Let’s make it a follow-up issue. Maybe an easier improvement can be found also. My worry is that users won’t notice at all that there might be more keys than are shown (especially since the list does not fill the whole widget/screen if the terminal is larger). Maybe the presence of more keys can be detected (by requesting slightly more than are shown?) and in this case add some sort of marker in the list? Also, could page-down and page-up maybe replace the k shortcut? But again, all of this can be a follow-up issue.

Could you address the other points?

@frcroth frcroth requested a review from fm3 October 16, 2023 13:31
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

🎉

@frcroth frcroth merged commit 09631f7 into master Oct 17, 2023
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.

3 participants