Skip to content

Conversation

@radrogow
Copy link
Contributor

@radrogow radrogow commented Nov 8, 2025

Also, UserContextFile, to work on live files

Closes #142

Copy link
Collaborator

@BrianPugh BrianPugh left a comment

Choose a reason for hiding this comment

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

Review: I think the code looks fantastic, thank you! While giving this a test, I instinctively wanted the following commands: {mkdir, mv, cat, cd}. What are your thoughts on adding these commands?


General comments that are not specific to this PR, but I noticed while reviewing:

  1. We should probably have UserContext be an abstract baseclass, and the current implementation should be something like UserContextInMemory. Currently the inheritance is kind of not correct. E.g. the existing UserContextWinDisk never calls any super(). But, it's probably best not risking breaking anything with a backward incompatible change for what is essentially an internal refactor.

@radrogow
Copy link
Contributor Author

radrogow commented Nov 8, 2025

What are your thoughts on adding these commands?

Done. As well as some more :)

About the "general comments" - I agree, with all points.
For eventual changes I'd suggest protocol instead of abc

Could you please publish a new version to pip when merged?
I'd like to use this code, but don't like to change my requirements.txt to git

@BrianPugh
Copy link
Collaborator

BrianPugh commented Nov 9, 2025

this all looks good! i'll merge/release soon! Thank you!

@BrianPugh BrianPugh merged commit a1477be into jrast:master Nov 9, 2025
21 checks passed
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.

Question: does cli/repl fit to this library?

2 participants