Message posting clean-up #4256
Merged
Message posting clean-up #4256
Conversation
This file contains 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
This is currently a failing test; work relating to Textualize#3869 should make this pass.
This is currently a failing test; work relating to Textualize#3869 should make this pass.
Fixed Collapsible for the purposes of Textualize#3869.
In support of work on Textualize#3869.
Until now it was possible, depending on the DOM, for this message to not bubble. This ensures that this won't be an issue. In support of work on Textualize#3869.
This will fail right at the moment; the fix follows. In support of work on Textualize#3869.
In support of work on Textualize#3869.
In aid of Textualize#3869. This will fail right now, this is a thing that needs to be fixed.
In aid of Textualize#3869. This ensures that when the dev makes a call on SelectionList.toggle, that SelectionList.SelectionToggled gets posted.
The call to load_text needed fixing for messages too; and actually setting TextArea.text calls this anyway. In support of Textualize#3869.
rodrigogiraoserrao
approved these changes
Mar 6, 2024
Comment on lines
704
to
710
| self._values = { | ||
| **self._values, | ||
| **{ | ||
| option.value: index | ||
| for index, option in enumerate(cleaned_options, start=self.option_count) | ||
| }, | ||
| } |
There was a problem hiding this comment.
Isn't .update better in terms of speed while being the same in terms of readability?
There was a problem hiding this comment.
Off the top of my head I don't know the speed difference; it feels like you are right, do you have a reference?
There was a problem hiding this comment.
I don't have a reference, no, it's just an educated guess.
I was just surprised you spelled this out 🤷
There was a problem hiding this comment.
I'd probably go with update. It would be faster, but doubt the difference would be measurable. It's more that update expresses intent better.
Mention them as breaking changes too; while this is an improvement that shouldn't have a negative impact on applications, it's still something developers should be aware of.
willmcgugan
approved these changes
Mar 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Assorted tweaks and fixes as fallout from the work done in #3869. Where possible/sensible extra tests have been added too.
The most significant change here, I think, is to
SelectionList, which needed a little bit of extra work because it became necessary to find back the index of a value. (while this is in review I'm going to give that part one last run over as I want to be double sure it's a good approach -- I think this also possibly either provides a "good" reason for #3908 or might actually make it less good)