search

pharo-vcs / iceberg

Iceberg is the main toolset for handling VCS in Pharo.
MIT License
133 stars 83 forks source link

Remove only the reverted diff entry rather than recompute the whole diff after revert (fixes #1745) #1782

Closed daniels220 closed 6 months ago

daniels220 commented 7 months ago

This feels pretty brittle and like the model objects should be able to do more of the work—for instance, the fact that I have to separately check isNoModification or: [isExtensionDefinition], because extension class definitions are always "added", yet have no content of their own, where a packaged class definition could be added as just an empty class. Also little things like the tree entries not having a "remove" API so I have to traverse up and ask the parent to remove the child, etc. Feels like the API here is under-developed, basically—and that makes sense because we ultimately could do things like update the diff in-place when a method is saved (after all, the only thing that can possibly change is the diff entry for that method...), which would probably drive the creation of a richer API here.

So—if this seems like a step forward, great. If you have any suggestions for making it less ugly and brittle without tackling a major revamp of the diff panel, I'm happy to put in a little more work.

Ducasse commented 7 months ago

Did you run the tests?

daniels220 commented 6 months ago

Which tests are you referring to? It doesn't look like there were any test failures in the CI build (the one Windows issue seems unrelated and I've seen before in other CI runs).

If what you mean is that I should write tests, fair. I I only just went looking and found that there are existing tests of the UI, as they're not loaded by default in a Pharo distribution image (reasonable enough), I had to load them from the repo. So I'll get to that when I have a chance...