Dart version update above 3 null safety #6

VladimirCores · 2023-08-05T09:34:58Z

Conform framework to the Dart 3 version.
Make all tests pass.

Example works.

Mr @cliffhall could you take a look?

…mandMap in Controller.

cliffhall

I've made a few requests, mainly around the refactoring of some params / vars to use underscores. Since this is a stylistic choice that is not backward compatible I ask that they be returned to their previous states. There are some other minor requests/comments as well.

lib/src/interfaces/IController.dart

cliffhall · 2023-08-09T14:57:22Z

lib/src/interfaces/INotification.dart

-    /**
-     * This [INotifications]'s [body]
-     */
-    void set body( dynamic bodyObject );


why are the accessors removed?

Because we have methods: getBody, setBody, the accessors looks excess, they duplicate functionality. And in my practice I always use these methods. What are the reasons for accessors existence?

Accessors are not duplicate functionality, they are alternate functionality. You can say myModel.body or myModel.getBody() whichever may be your preference. They exist as part of the original framework and all ports.

It's important to remember that personal stylistic choices should not distort a port. A port should hew as closely to the reference implementation as is practicable. This is why the ActionScript Developers Guide to PureMVC is generally useful do devs using most ports. Because the implementation and idioms are very similar across ports. If developers used to one port of the framework find that the same idioms they are used to are present in another port, they get to joy quicker. If they arrive here and find that things are different owing to stylistic choices, well, it all becomes a bit loosey-goosey. The promise of easy portability breaks down. If this one thing is arbitrarily different, what others might be?

cliffhall · 2023-08-09T14:59:40Z

lib/src/core/Controller.dart

+  Controller(String key) {
+    if (instanceMap.containsKey(key)) throw MultitonErrorControllerExists();
+    _multitonKey = key;
+    instanceMap[_multitonKey] = this;


Change _multitonKey back to multitonKey.

Setter added for every places near getter.
But why multitonKey could be mutable? Can't imagine use case.

Ok, this one is my bad. I failed to realize that Dart had promoted the underscore as an alternative to having protected or private keyword, as was present in the AS3 reference.

So in this case, we legit have to break from the reference implementation and do something different. This means all the Multiton actors need this underscore for the multitonKey property. Please do an exhaustive search to make sure all references are replaced, because I recall that in the original PR, one of the classes still had a loose multitonKey ref without the underscore on it.

cliffhall · 2023-08-09T15:00:24Z

lib/src/core/Model.dart

-    proxyMap = new Map<String,IProxy>();
+  Model(String key) {
+    if (instanceMap.containsKey(key)) throw MultitonErrorModelExists();
+    _multitonKey = key;


Change _multitonKey back to multitonKey.

Setter added for every places near getter.

cliffhall · 2023-08-09T15:26:53Z

test/Test_Observer.dart


        // Make sure caller and callback were set
-        expect( observer.getNotifyMethod(), isNotNull  );
-        expect( observer.notifyMethod, isNotNull  );


why was this test removed?

cliffhall · 2023-08-09T15:27:20Z

test/Test_Observer.dart


        // Make sure callback method was set
-        expect( observer.getNotifyMethod(), isNotNull );
-        expect( observer.notifyMethod, isNotNull );


why was this accessor test removed?

I might guess that accessors are needed to keep original version of the property in case something overrides them.

cliffhall · 2023-08-09T15:27:33Z

test/Test_Observer.dart


        // Make sure caller was set
-        expect( observer.getNotifyContext(), isNotNull );
-        expect( observer.getNotifyContext(), same(this) );
-        expect( observer.notifyContext, isNotNull );


why was this test removed?

I removed direct access to the value of notifyContext because I though its excess and creates ambiguity in understanding of their use (as we already have methods). But I reverted the remove, and the test as well.

cliffhall · 2023-08-09T15:32:58Z

test/Unit_Tests.dart

+part 'Test_Facade.dart';
+part 'Test_MacroCommand.dart';
+part 'Test_Mediator.dart';
+part 'Test_Model.dart';
 // PureMVC Unit Tests


why did this comment get moved to the middle of the list of tests?

Not sure how ... but its corrected, and comment moved to the top of "declaration" of parts.

cliffhall · 2023-08-09T15:34:38Z

test/Unit_Tests.dart

-  void write(String message) {
-    document.querySelector('#status').innerHtml = message;
-  }
+  // void onTestResult(TestCase testCase) {


why are these output messages commented out? If no longer needed, we can just remove them. Are they made redundant by the test runner?

They are not needed for test run from IDE (Android Studio) and this commented out code removed.

cliffhall

Another small request. Could you please update the VERSION and README files to add notes about the changes, and increment this to 2.0.7?

VladimirCores · 2023-08-10T04:24:27Z

@cliffhall I read your messages, and will answer (and correct) them I hope soon, have some urgent business. Thanks for your collaboration on this! : )

… by auto-replace.

…not needed for run of the tests.

…tter in View, Model and Controller.

…(MacroCommand)

…structor.

…nd observer.notifyContext.

VladimirCores · 2023-08-14T06:25:37Z

@cliffhall I will review this PR once again and make changes you requested.

cliffhall

There were a lot more changes this time. Moving target. Unfortunately, I'm requesting most of them be reverted under three major themes:

First is my bad. I didn't realize _ was elevated to language level 'private' and no longer just an idiom here. If that's what it takes to make the property private, then so be it, _ on private vars throughout.
For maximum portability, we eschew the local syntactic sugar of a language wherever possible and favor simple, widely supported idioms.

So for instance, in a collection where you're invoking containsKey, you're relying on a Dart-specific feature. While myMap.containsKey('stuff') is 100% how you might go about it in your application, in a portable framework myMap['stuff'] is the favored approach.

That's important to do even in ports of the reference implementation, because someone who uses this port might decide to port it to their new favorite language. If they're going from a very Dart-flavored implementation, then they need to seek equivalent affordances in the target language and that amounts to friction for the porting developer and a reduced likelihood that they will complete the port.

With regards to portability, PureMVC took a hint from C, which is so dirt simple it exists for nearly every processor in the world.

In some places, refactors have been done that, while possibly making more sense or being slightly more efficient, should not have happened. A particularly sensitive one is notifyObservers. It took a long time to get that one working properly. Lots of community feedback led to a stable implementation. I'd like to keep all of the method implementations as close to the original as is allowed by the language. By doing so, we leverage years of troubleshooting and improvement to get the expected behaviors we see in the ports today. As a result we usually don't see new issues cropping up after a port. Simple refactors can lead to bugs that lie dormant because a particular weird use-case has not been tried, as was the case with notifyObservers.

cliffhall · 2023-08-17T16:55:33Z

lib/src/core/Controller.dart

+    if (instanceMap.containsKey(key)) throw MultitonErrorControllerExists();
+    _multitonKey = key;
+    instanceMap[_multitonKey] = this;
+    commandMap = Map<String, Function>();


Based on the above comment, commandMap and several other properties of the Multitons should be underscored as well. check the AS3 Reference for props that were protected or private.

cliffhall · 2023-08-17T16:57:15Z

lib/src/core/Controller.dart

-  {
-    view = View.getInstance( multitonKey );
+  void initializeController() {
+    view = View.getInstance(multitonKey)!;


Here's one where it needs to be underscored but wasn't. It's going to be important to sweep all the classes and make sure all the props intended to be private/protected have underscores everywhere they're referenced.

cliffhall · 2023-08-17T17:00:26Z

lib/src/core/Controller.dart

-    return instanceMap[ key ];
+  static IController? getInstance(String? key) {
+    if (key == null || key == "") return null;
+    if (instanceMap.containsKey(key)) {


Why is this logic refactored? It may do the same thing in the end, but please, if possible, stick with the existing logic implementation, which was inherited from the AS3 port, which had years of improvement associated with it to eliminate hard to detect bugs. While one implementation might be as good as another, it's where new bugs can creep in. We'd like to make sure that we lean on the well-tested reference wherever we can, departing from it only when we have no other choice.

cliffhall · 2023-08-17T17:04:08Z

lib/src/core/Controller.dart

-    Function commandFactory = commandMap[ note.getName() ];
-    if ( commandFactory == null ) return;
+  void executeCommand(INotification note) {
+    final noteName = note.getName();


Another unnecessary departure from the original.

cliffhall · 2023-08-17T18:48:03Z

lib/src/core/Controller.dart

-    if ( commandMap[ noteName ] == null ) {
-      view.registerObserver( noteName, new Observer( executeCommand, this ) );
+  void registerCommand(String noteName, Function commandFactory) {
+    if (!hasCommand(noteName)) {


Stick with the original implementation wherever possible.

cliffhall · 2023-08-17T20:00:46Z

lib/src/patterns/facade/Facade.dart


  // The [IFacade] Multiton instanceMap.
-  static Map<String,IFacade> instanceMap;
+  static Map<String, IFacade> instanceMap = Map<String, IFacade>();


why is this being initialzed?

cliffhall · 2023-08-17T20:02:00Z

lib/src/patterns/observer/Notification.dart

+   *
+   * -  Returns [String] - the type of the [INotification].
+   */
+  String? getType() {


Again, not sure what the ? is for in this context, can we drop it throughout?

cliffhall · 2023-08-17T20:03:57Z

lib/src/patterns/observer/Notification.dart

+  /**
+   * This [INotifications]'s [name]
+   */
+  final String _name;


why are you adding final to _name but not _type? I think dropping it from both is what's needed.

cliffhall · 2023-08-17T20:04:43Z

lib/src/patterns/observer/Notification.dart

+  /**
+   * This [INotifications]'s [type]
+   */
+  String? _type;


questioning the ?. I think. we can drop throughout (in this context)

cliffhall · 2023-08-17T20:07:52Z

test/Unit_Tests.dart

-    document.querySelector('#status').innerHtml = message;
-  }
+  // void onTestResult(TestCase testCase) {
+  //   write("${testCase.result}  ${testCase.currentGroup}");


Can we remove these comments?

Vladimir Minkin added 4 commits August 4, 2023 20:00

Linter rules config added

6c85d0c

Conform framework to the Dart 3 version. Make tests pass.

908a7f5

Use native method containsKey to check existence of the value for com…

41f0d38

…mandMap in Controller.

Keyword new removed from code.

fabf4aa

cliffhall requested changes Aug 9, 2023

View reviewed changes

cliffhall mentioned this pull request Aug 9, 2023

Please update analyzer to the vesion >= 0.32.4 #5

Closed

cliffhall requested changes Aug 9, 2023

View reviewed changes

Vladimir Minkin added 2 commits August 13, 2023 08:10

Setter for multitonKey returned back

e40f5d2

Keyword new returned to the comments in places where it was removed…

d344648

… by auto-replace.

VladimirCores force-pushed the dart-version-update-above-3-null-safety branch from 198a5c1 to d344648 Compare August 13, 2023 05:18

Vladimir Minkin added 14 commits August 13, 2023 08:32

Observer setter corrected - proper target for value - _notifyContext

2775755

Comments parts in Unit_Tests.dart rearranged to top.

a1f0725

Library 'dart:html' and related use of it removed from Unit_Tests as …

f42377d

…not needed for run of the tests.

Add access modifiers for notifyContext and notifyMethod in Observer

5457596

Unnecessary comment on registerProxy in Model removed.

433aaac

Initialize multitoneKey with constructor input value key through se…

f1c9c4e

…tter in View, Model and Controller.

Revert back test on observer.notifyContext

eec1bd8

Corrections in comments - [_multitonKey] => [multitonKey]

c8d0de4

Use getter for multitonKey when commandInstance.initializeNotifier …

f7220a0

…(MacroCommand)

Setting and accessing multitonKey from access modifiers in Facade con…

bf4cab2

…structor.

Use access modifiers for multitonKey

d14e870

Open properties of Notification - name, body, type

1c82974

Comments added from previous versions of IObserver

db0122b

Return back tests for properties of observer: observer.notifyMethod a…

357356b

…nd observer.notifyContext.

VladimirCores force-pushed the dart-version-update-above-3-null-safety branch from 9d9f4bd to 357356b Compare August 13, 2023 06:34

README and VERSION update

bdfd798

cliffhall requested changes Aug 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dart version update above 3 null safety #6

Dart version update above 3 null safety #6

VladimirCores commented Aug 5, 2023 •

edited

Loading

cliffhall left a comment

cliffhall Aug 9, 2023

VladimirCores Aug 13, 2023

cliffhall Aug 17, 2023 •

edited

Loading

cliffhall Aug 9, 2023

VladimirCores Aug 13, 2023 •

edited

Loading

cliffhall Aug 17, 2023 •

edited

Loading

cliffhall Aug 9, 2023

VladimirCores Aug 13, 2023

cliffhall Aug 9, 2023

cliffhall Aug 9, 2023 •

edited

Loading

VladimirCores Aug 13, 2023

cliffhall Aug 9, 2023

VladimirCores Aug 13, 2023

cliffhall Aug 9, 2023 •

edited

Loading

VladimirCores Aug 13, 2023

cliffhall Aug 9, 2023

VladimirCores Aug 13, 2023

cliffhall left a comment •

edited

Loading

VladimirCores commented Aug 10, 2023

VladimirCores commented Aug 14, 2023

cliffhall left a comment •

edited

Loading

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

cliffhall Aug 17, 2023

Dart version update above 3 null safety #6

Are you sure you want to change the base?

Dart version update above 3 null safety #6

Conversation

VladimirCores commented Aug 5, 2023 • edited Loading

cliffhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffhall Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladimirCores Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

cliffhall Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffhall Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffhall Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffhall left a comment • edited Loading

Choose a reason for hiding this comment

VladimirCores commented Aug 10, 2023

VladimirCores commented Aug 14, 2023

cliffhall left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladimirCores commented Aug 5, 2023 •

edited

Loading

cliffhall Aug 17, 2023 •

edited

Loading

VladimirCores Aug 13, 2023 •

edited

Loading

cliffhall Aug 17, 2023 •

edited

Loading

cliffhall Aug 9, 2023 •

edited

Loading

cliffhall Aug 9, 2023 •

edited

Loading

cliffhall left a comment •

edited

Loading

cliffhall left a comment •

edited

Loading