Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

daniels220
Copy link
Contributor

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
Copy link
Collaborator

Ducasse commented Dec 16, 2023

Did you run the tests?

@daniels220
Copy link
Contributor Author

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...

Copy link
Member

@guillep guillep left a comment

Choose a reason for hiding this comment

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

Looks good enough, let's try it! Thanks @daniels220 !

@guillep guillep merged commit eb8661b into pharo-vcs:dev-2.0 Jan 18, 2024
2 of 3 checks passed
@daniels220 daniels220 deleted the remove-only-reverted-entry branch January 18, 2024 17:18
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