Manuscript (Dossier [Issue]20591) _Inbox - WideCharacterSet is not a subclass of CharacterSet

Nicolas Cellier edited Case 20591: WideCharacterSet is not a subclass of CharacterSet: Enhancement in  _Inbox: Not Spam  •  You are subscribed to this case Thanks for adding information... What does the title really tells? If I assert: "LargeInteger is not a subclass of SmallInteger". You might answer, well OK, it does not necessarily need to be so... Direct subclassing is an option, deriving from a common superclass is yet another. What about CharacterSetComplement? Not every trivial change is worth an explanation. But here, changes are not trivial and deserve more than a title... The PR says 17 files changed, 739 lines added 191 removed. So at first sight, it's not just about an economy of code as we could have expected from a better code factorisation... Or the PR is only vaguely related... But then from a quality POV, there's a mismatch. By inspecting the PR, I see unrelated changes that we can put on the account of tonel/git migration. But that's not all. I also deduce that the primary goal was to implement union:/intersection: and I don't see anything about that in this report. Why? Then indeed, implementing those methods might rapidly lead to the conclusion that common code should be better factored - which becomes a secondary goal. About implementation merits, inheriting means reusing instance variables... By web browsing, I understand that the wide subclass is reusing the map ivar but is changing its semantics! And the subclass adds a byteArrayMap ivar that has the semantics of super map. I think that reusing the super "byteArrayMap" (map) for byte characters and adding a new ivar for wide characters is indeed a clever idea that is indeed justifying the subclassing option (a WideCharacterSet is a (Byte)CharacterSet plus something). But IMO: - inst. var. reuse should be the other way around. - anything CLEVER is asking for more exposure. Still IMO, the Size/Large/Small is a nice exercize in style. Since they are also in our global namespace, a discussion of its merits versus alternatives would be more than welcome. I've looked in Squeak where it's easier for me to contribute, but which has essentially the same code base. And I came to the conclusion that CharacterSetComplement >> select:/reject: was badly broken and would require revisiting. I could solve the problem with yet another character set flavour, a LazyCharacterSet... This might change the deal (and thus some of the design decisions too). But if the subject is not raised/discussed anywhere, there's zero chance to share/experiment/confront. I would recommend - exposing the primary goals, (the XY problem) - explaining main design decisions and/or opening discussions for non trivial changes (mailing list, IRC channels, whatever) - referencing the eventual places where design discussion took place so as to share the knowledge and avoiding reviewers to ask already answered questions. - splitting the commits into smaller pieces, so as to enable reviewing (we could already do that with MC but it's more straight with explicitely named branches like in git) Priority: 5 – Fix If Time Status: Work Needed Assigned to: Pavel Krivanek Milestone: Pharo7.0 Go to Case No longer need updates? Unsubscribe from this case. Don't want Manuscript notifications anymore? Update your preferences.

Modifié par Nicolas Cellier. N° de dossier : 20591 Intitulé : WideCharacterSet is not a subclass of CharacterSet État: Work Needed Catégorie: Enhancement Projet : _Inbox Zone : Not Spam Priorité : 5 - Fix If Time Version : Pharo7.0: 01/09/2017 (Écoulé) Attribué à: Pavel Krivanek URL : https://pharo.manuscript.com/f/cases/20591 Thanks for adding information... What does the title really tells? If I assert: "LargeInteger is not a subclass of SmallInteger". You might answer, well OK, it does not necessarily need to be so... Direct subclassing is an option, deriving from a common superclass is yet another. What about CharacterSetComplement? Not every trivial change is worth an explanation. But here, changes are not trivial and deserve more than a title... The PR says 17 files changed, 739 lines added 191 removed. So at first sight, it's not just about an economy of code as we could have expected from a better code factorisation... Or the PR is only vaguely related... But then from a quality POV, there's a mismatch. By inspecting the PR, I see unrelated changes that we can put on the account of tonel/git migration. But that's not all. I also deduce that the primary goal was to implement union:/intersection: and I don't see anything about that in this report. Why? Then indeed, implementing those methods might rapidly lead to the conclusion that common code should be better factored - which becomes a secondary goal. About implementation merits, inheriting means reusing instance variables... By web browsing, I understand that the wide subclass is reusing the map ivar but is changing its semantics! And the subclass adds a byteArrayMap ivar that has the semantics of super map. I think that reusing the super "byteArrayMap" (map) for byte characters and adding a new ivar for wide characters is indeed a clever idea that is indeed justifying the subclassing option (a WideCharacterSet is a (Byte)CharacterSet plus something). But IMO: - inst. var. reuse should be the other way around. - anything CLEVER is asking for more exposure. Still IMO, the Size/Large/Small is a nice exercize in style. Since they are also in our global namespace, a discussion of its merits versus alternatives would be more than welcome. I've looked in Squeak where it's easier for me to contribute, but which has essentially the same code base. And I came to the conclusion that CharacterSetComplement >> select:/reject: was badly broken and would require revisiting. I could solve the problem with yet another character set flavour, a LazyCharacterSet... This might change the deal (and thus some of the design decisions too). But if the subject is not raised/discussed anywhere, there's zero chance to share/experiment/confront. I would recommend - exposing the primary goals, (the XY problem) - explaining main design decisions and/or opening discussions for non trivial changes (mailing list, IRC channels, whatever) - referencing the eventual places where design discussion took place so as to share the knowledge and avoiding reviewers to ask already answered questions. - splitting the commits into smaller pieces, so as to enable reviewing (we could already do that with MC but it's more straight with explicitely named branches like in git) Vous êtes abonné(e) à ce dossier. Si vous ne souhaitez plus recevoir de notifications automatiques à l'avenir, désabonnez-vous (https://pharo.manuscript.com/default.asp?pre=preUnsubscribe&pg=pgEditBug&command=view&ixBug=20591) de ce dossier.
_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
https://lists.gforge.inria.fr/mailman/listinfo/pharo-bugtracker