alexeyinkin commented on code in PR #24283:
URL: https://github.com/apache/beam/pull/24283#discussion_r1034392658
##########
playground/frontend/playground_components/lib/src/models/example.dart:
##########
@@ -49,8 +53,11 @@ class Example extends ExampleBase {
required this.logs,
required this.outputs,
required this.source,
+ ExampleLoadingDescriptor? descriptor,
this.graph,
- }) : super(
+ }) : descriptor =
+ descriptor ?? StandardExampleLoadingDescriptor(path: example.path),
Review Comment:
We should make no such assumption. `path` here is just a poorly chosen name
from the age when we only had standard examples. Now it can be used for
anything.
I am thinking of these options:
- To make the descriptor required.
- To default to `ContentExampleLoadingDescriptor` which is safest one
although the least shareable one.
##########
playground/frontend/playground_components/test/src/controllers/snippet_editing_controller_test.dart:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter_test/flutter_test.dart';
+import
'package:playground_components/src/controllers/snippet_editing_controller.dart';
+import 'package:playground_components/src/models/sdk.dart';
+
+import '../common/examples.dart';
+
+void main() {
+ group(
+ 'Snippet editing controller',
+ () {
+ test(
+ 'Returns standard descriptor if code has not been changed',
+ () {
+ final controller = SnippetEditingController(sdk: Sdk.python);
+ controller.selectedExample = exampleMock1;
+
+ final descriptor = controller.getLoadingDescriptor();
+
+ expect(descriptor.toJson(), {'example':
'SDK_PYTHON/Category/Name1'});
+ },
+ );
+
+ test(
+ 'Returns content descriptor if code has been changed',
+ () {
+ final controller = SnippetEditingController(sdk: Sdk.python);
+ controller.selectedExample = exampleMock1;
+
+ controller.codeController.value = const TextEditingValue(text:
'ex4');
+ final descriptor = controller.getLoadingDescriptor();
+
+ expect(descriptor.toJson(), {
+ 'sdk': 'python',
+ 'content': 'ex4',
+ 'name': 'Example X1',
+ 'complexity': 'basic',
+ });
Review Comment:
It is sufficient to test that it is `ContentExampleLoadingDescriptor` with
all the fields. Otherwise this test has one more reason to change if the fields
are renamed in the descriptor.
The descriptor's ability to be recovered in `toJson` -> `tryParse` should be
tested separately.
##########
playground/frontend/lib/pages/embedded_playground/components/embedded_actions.dart:
##########
@@ -58,16 +59,24 @@ class EmbeddedActions extends StatelessWidget {
void _openStandalonePlayground(PlaygroundController controller) {
// The empty list forces the parsing of EmptyExampleLoadingDescriptor
// and prevents the glimpse of the default catalog example.
+
+ final descriptor = controller.getLoadingDescriptor();
+ final urlDescriptor = descriptor.where((d) => d.canBePassedByUrl);
+ final jsDescriptor = descriptor.where((d) => !d.canBePassedByUrl);
+
+ final json = jsonEncode(urlDescriptor.toJson()['descriptors']);
Review Comment:
This way, if `ExamplesLoadingDescriptor` contains anything else but
`.descriptors`, it is lost and we have a false sense that it is not. And it
does contain `lazyLoadDescriptors` although this is not used in the embedded
playground.
Also this way `['descriptors']` is compile-time unsafe. If it is renamed, we
sure will forget to update it here.
I suggest working with `descriptor.descriptors` directly. This way:
1. We do not have the false sense of completeness when passing the entire
`jsDescriptor` in the message.
2. We cut one level of indirection of `ExamplesLoadingDescriptor.where`.
3. We eliminate the confusion of `ExamplesLoadingDescriptor.where` losing
data.
##########
playground/frontend/playground_components/lib/src/models/example_loading_descriptors/example_loading_descriptor.dart:
##########
@@ -28,4 +29,6 @@ abstract class ExampleLoadingDescriptor with EquatableMixin {
final ExampleViewOptions viewOptions;
Map<String, dynamic> toJson() => throw UnimplementedError();
+
+ bool get canBePassedByUrl => this is! ContentExampleLoadingDescriptor;
Review Comment:
`canBePassedInUrl`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
