Unify history operations under a struct. Add traits to make different history approaches feasible #92
Conversation
nixypanda
changed the title
There was a problem hiding this comment.
Added some comments. The points you have about duplication and removing clones are fine, but to my mind these could be part of an optimization pass you do later.
Are you feeling comfortable with the general direction in the PR? I noticed in the design, you also mention things like fuzzy searching, which it looked like we could build onto this PR.
- Implement all the trait for FileBackedHistory
nixypanda
force-pushed
the
history-experiments
branch
from
0c0c8a0
to
3d847d1
Compare
I also agree with the point of revisiting to clean up duplication and do optimizations if the general direction of the PR is good. (which I hope is the case here) I am pretty comfortable with how the unification worked out overall. I did not and fuzzy search as part of the PR as I wanted to not mix adding feature and clean up of code (i.e. merging both histories). I think is this something that we can take up at a later stage. |
Not using this in line buffer currently as line buffer makes use of a lot of custom logic
|
@jonathandturner I have marked conversations resolved by adding a comment with the rationale behind it. If there is anything that you would me to take care of please mark the conversation unresolved. P.S. I am not super sure about the dev flow that you have in this repo, like is it ok that I (PR author) marks the conversation resolved or should the reviewer be the only one that does so. |
|
@sherubthakur - yup, it's totally okay to mark things as resolved if you think they're resolved. Giving this PR another look now |
|
This looks good to me. |
Resolves #86 and #55