DRAFT: refactor(entities,resources): Decouple parsing from entities hook into a separate serializer class #1671

mmelko · 2024-11-28T15:23:29Z

create CamelResourceSerializer interface and YamlCamelResourceSerializer implementantion
create CamelResourceFactory and CamelKResourceFactory to creating new resources
move parsing out of the entities hook

lordrip · 2024-11-29T09:41:10Z

Thanks @mmelko for putting this together. I'm turning this PR into a draft for now, so we don't consume Chromatic credits until we're ready to merge 😄

lordrip

Looks good @mmelko 💪 , let's try to tackle the open comments first

lordrip · 2024-11-29T11:45:59Z

packages/ui/src/hooks/entities.ts

@@ -15,7 +16,6 @@ import { EventNotifier } from '../utils';
 * ```
 * The regular expression should match the first three lines
 */
-const COMMENTED_LINES_REGEXP = /^\s*#.*$/;


[minor] Please let's move this comment as well

packages/ui/src/serializers/yaml-camel-resource-serializer.ts

packages/ui/src/models/camel/camel-k-resource-factory.ts

packages/ui/src/models/camel/camel-k-resource.ts

lordrip · 2024-11-29T12:20:48Z

packages/ui/src/models/camel/camel-route-resource.ts

+  getSerializer(): CamelResourceSerializer {
+    return this.serializer;
+  }
+  setSerializer(serializer: CamelResourceSerializer): void {
+    this.serializer = serializer;
+  }
+


Suggested change

getSerializer(): CamelResourceSerializer {

return this.serializer;

}

setSerializer(serializer: CamelResourceSerializer): void {

this.serializer = serializer;

}

packages/ui/src/models/camel/camel-route-resource.ts

lordrip · 2024-11-29T12:21:52Z

packages/ui/src/models/camel/kamelet-resource.test.ts

-  describe('comments', () => {
-    it('should set and get comments', () => {
-      const resource = new KameletResource();
-      resource.setComments(['a', 'b']);
-      expect(resource.getComments()).toEqual(['a', 'b']);
-    });
-  });


Same here, can we create a YAML containing some comments and check they are preserved?

packages/ui/src/models/camel/kamelet-resource.ts

lordrip · 2024-11-29T12:22:50Z

packages/ui/src/models/camel/pipe-resource.test.ts

-      resource.setComments(['a', 'b']);
-      expect(resource.getComments()).toEqual(['a', 'b']);
-    });
-  });
 });


Same as before :D

mmelko · 2024-11-29T13:36:48Z

@lordrip thanks for the review! Updated code coming soon.

…o separate serializer class * create CamelResourceSerializer interface and YamlCamelResourceSerializer implementantion * create CamelResourceFactory and CamelKResourceFactory to creating new resources * move parsing out of the entities hook

codecov · 2024-12-02T13:14:12Z

Codecov Report

Attention: Patch coverage is 76.00000% with 24 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@78a9529). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...i/src/serializers/xml-camel-resource-serializer.ts	30.00%	7 Missing ⚠️
.../src/serializers/yaml-camel-resource-serializer.ts	80.00%	6 Missing ⚠️
packages/ui/src/models/camel/camel-k-resource.ts	50.00%	5 Missing ⚠️
...ckages/ui/src/models/camel/camel-route-resource.ts	61.53%	5 Missing ⚠️
...ages/ui/src/models/camel/camel-resource-factory.ts	91.66%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1671   +/-   ##
=======================================
  Coverage        ?   78.96%           
  Complexity      ?      365           
=======================================
  Files           ?      464           
  Lines           ?    14541           
  Branches        ?     2780           
=======================================
  Hits            ?    11483           
  Misses          ?     2789           
  Partials        ?      269

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lordrip · 2024-12-12T09:00:30Z

packages/ui/src/models/camel/camel-route-resource.ts

-    if (!json) return;
-    const rawEntities = Array.isArray(json) ? json : [json];
-    this.entities = rawEntities.reduce((acc, rawItem) => {
+  constructor(code?: unknown, serializer?: CamelResourceSerializer) {


Let's try to use the CamelYamlDsl type

Suggested change

constructor(code?: unknown, serializer?: CamelResourceSerializer) {

constructor(code: CamelYamlDsl = [], serializer: CamelResourceSerializer = new YamlCamelResourceSerializer()) {

Please try to find a better name than the existing code 🙏 📦

lordrip · 2024-12-12T09:20:40Z

packages/ui/src/serializers/yaml-camel-resource-serializer.ts

+   * ```
+   * The regular expression should match the first three lines
+   */
+  COMMENTED_LINES_REGEXP = /^\s*#.*$/;


Maybe it is better to use static for this.

lordrip · 2024-12-12T09:23:21Z

packages/ui/src/serializers/camel-resource-serializer.ts

+export interface CamelResourceSerializer {
+  parse: (code: string) => unknown;
+  serialize: (resource: CamelResource) => string;
+}


Suggested change

}

getComments: () => string[];

setComments: (comments: string[]) => void;

}

lordrip · 2024-12-12T09:23:51Z

packages/ui/src/models/camel/camel-route-resource.ts

+    return this.serializer;
+  }
+  setSerializer(serializer: CamelResourceSerializer): void {
+    this.serializer = serializer;


Suggested change

this.serializer = serializer;

// Get the existing comments and move them to the new serializer

serializer.setComments(this.serializer.getComments());

this.serializer = serializer;

mmelko · 2024-12-12T13:36:30Z

@lordrip I incorporated your suggestions and also added types into the CamelResourceFactory, please check.

…f unknown update tests to reflect types

sonarcloud · 2024-12-12T17:14:30Z

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.0% Duplication on New Code

See analysis details on SonarQube Cloud

mmelko requested a review from lordrip November 29, 2024 08:31

lordrip marked this pull request as draft November 29, 2024 09:40

lordrip reviewed Nov 29, 2024

View reviewed changes

mmelko force-pushed the serializers branch from 050d994 to 3785faf Compare December 1, 2024 19:31

mmelko force-pushed the serializers branch from 3785faf to 3a2e5de Compare December 2, 2024 10:40

reflect PR comments

3a34678

mmelko force-pushed the serializers branch from 3a2e5de to 3a34678 Compare December 2, 2024 13:09

lordrip reviewed Dec 12, 2024

View reviewed changes

fix(CamelResourceFactory): use types for the CamelResources instead o…

020af1b

…f unknown update tests to reflect types

mmelko force-pushed the serializers branch from 21a50ff to 020af1b Compare December 12, 2024 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRAFT: refactor(entities,resources): Decouple parsing from entities hook into a separate serializer class #1671

DRAFT: refactor(entities,resources): Decouple parsing from entities hook into a separate serializer class #1671

mmelko commented Nov 28, 2024

lordrip commented Nov 29, 2024

lordrip left a comment

lordrip Nov 29, 2024

lordrip Nov 29, 2024

lordrip Nov 29, 2024

lordrip Nov 29, 2024

mmelko commented Nov 29, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

lordrip Dec 12, 2024 •

edited

Loading

lordrip Dec 12, 2024

lordrip Dec 12, 2024

lordrip Dec 12, 2024

lordrip Dec 12, 2024

mmelko commented Dec 12, 2024

sonarcloud bot commented Dec 12, 2024

	constructor(code?: unknown, serializer?: CamelResourceSerializer) {
	constructor(code: CamelYamlDsl = [], serializer: CamelResourceSerializer = new YamlCamelResourceSerializer()) {

-}
+  getComments: () => string[];
+  setComments: (comments: string[]) => void;
+}

DRAFT: refactor(entities,resources): Decouple parsing from entities hook into a separate serializer class #1671

Are you sure you want to change the base?

DRAFT: refactor(entities,resources): Decouple parsing from entities hook into a separate serializer class #1671

Conversation

mmelko commented Nov 28, 2024

lordrip commented Nov 29, 2024

lordrip left a 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

mmelko commented Nov 29, 2024

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

lordrip Dec 12, 2024 • 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

mmelko commented Dec 12, 2024

sonarcloud bot commented Dec 12, 2024

Quality Gate passed

codecov bot commented Dec 2, 2024 •

edited

Loading

lordrip Dec 12, 2024 •

edited

Loading