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.

Sign up for GitHub

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

Jump to bottom

Implement loading changes into the image #85

Merged
merged 11 commits into from
Jun 26, 2020
Merged

Implement loading changes into the image #85

merged 11 commits into from
Jun 26, 2020

Conversation

MarcelGarus
Copy link
Member

@MarcelGarus MarcelGarus commented Jun 16, 2020 •
edited by SilvanVerhoeven
Loading

Merge #87 first

strawberriesandcheese and others added 2 commits June 17, 2020 11:07
@strawberriesandcheese @SilvanVerhoeven
Remove unnecessary spaces
0e8031f 
Co-authored-by: SilvanVerhoeven <44174681+SilvanVerhoeven@users.noreply.github.com>
@strawberriesandcheese @SilvanVerhoeven
Remove unnecessary spaces
3ddbc11 
Co-authored-by: SilvanVerhoeven <44174681+SilvanVerhoeven@users.noreply.github.com>
@MarcelGarus @strawberriesandcheese
Update packages/ChangesReloaded-Core.package/CRBrowser.class/instance…
4896b1f 
…/loadSelectedChanges.st

Co-authored-by: strawberriesandcheese <ulrike.herwig@student.hpi.de>
@MarcelGarus
Copy link
Member Author

Ooops, I didn't think this through. 😬
I thought we could remove the changeRecord, because we implement the noSuchMethodError: to delegate methods to the changeRecord. But this only works as soon as the other PR is merged – since this is an independent PR for now, we should probably revert my last change.
We can (and should) still update the code as soon as the other PR is merged, but that's probably not the only place where we could do things more elegantly, so I suggest we do that at a later point in time.

@strawberriesandcheese
Copy link
Contributor

strawberriesandcheese commented Jun 17, 2020 •
edited
Loading

ok #87 needs to be merged first but this branch still doesn't fail in ci because we don't test it. this is fine

strawberriesandcheese
strawberriesandcheese requested changes Jun 21, 2020
View reviewed changes
Copy link
Contributor

@strawberriesandcheese strawberriesandcheese left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR breaks our coverage reporting. Also Grit and I reviewed this together.

Unrelated Ulrike note: "history" is kinda weird in the grand scheme of our naming scheme. It's the sorted list of ChangeListItems for a HistoryListItem but we usually call those things List/Item stuff (as you can see by these examples alone). It kinda feels out of place. Also I don't feel like it's very descriptive/understandable.

.smalltalk.ston Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@SilvanVerhoeven SilvanVerhoeven mentioned this pull request Jun 22, 2020
Merged
@strawberriesandcheese strawberriesandcheese merged commit 331e85f into dev Jun 26, 2020
@strawberriesandcheese strawberriesandcheese deleted the 49-load-selected-changes branch June 26, 2020 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SilvanVerhoeven SilvanVerhoeven SilvanVerhoeven left review comments

@strawberriesandcheese strawberriesandcheese strawberriesandcheese approved these changes

@gritfessel gritfessel Awaiting requested review from gritfessel

Assignees

@MarcelGarus MarcelGarus

@strawberriesandcheese strawberriesandcheese

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MarcelGarus @strawberriesandcheese @SilvanVerhoeven @gritfessel