AttSync: Disconnect when attachment target is null #4538
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Octol1ttle
changed the title
modmuss50
reviewed
Mar 31, 2025
Member
modmuss50
left a comment
modmuss50
left a comment
There was a problem hiding this comment.
This needs some form of testing, ideally automated if not a command that tried to do something funny would be fine.
...tachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentChange.java
Outdated
Show resolved
Hide resolved
...tachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentChange.java
Outdated
Show resolved
Hide resolved
Member
|
Hey @Octol1ttle no pressure, just wondering if you are planning on finishing this off, its so close to being done. Im happy to help/take over if you like. |
Contributor
Author
|
Sorry, this PR completely left my memory. I resolved the review comments, but I have zero idea on how to do the testing bit 😅 |
Member
|
Awesome, thanks for that. I'll take a look a testing, worse case I'll just do some manual testing. |
Member
|
I have added a basic unit test, and marked the file to be translated. |
modmuss50
approved these changes
Apr 9, 2025
modmuss50
added
the
status: last call
modmuss50
added
the
status: merge me please
modmuss50
added a commit
that referenced
this pull request
Apr 13, 2025
* Add descriptive error message when receiving attachment changes for unknown targets * fix line breaks... * fix checkstyle * use StringJoiner, disconnect on error * Add pretty error message * Make error message translatable, add AttachmentSyncException * Break out of attachment application loop if there's an error * Add a simple unit test for applying to an invalid target. * Add lang file to be translated. * woops --------- Co-authored-by: modmuss50 <[email protected]> (cherry picked from commit f94390d)
modmuss50
added a commit
that referenced
this pull request
Apr 20, 2025
* Add descriptive error message when receiving attachment changes for unknown targets * fix line breaks... * fix checkstyle * use StringJoiner, disconnect on error * Add pretty error message * Make error message translatable, add AttachmentSyncException * Break out of attachment application loop if there's an error * Add a simple unit test for applying to an invalid target. * Add lang file to be translated. * woops --------- Co-authored-by: modmuss50 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Example:
