feat: add the flag for disabling optimizations #1447

helios2003 · 2024-05-22T18:49:27Z

Description
Now the disableOptimizationFor parameter can also be provided in the optimized document.

Related issue(s)
Fixes #1432

Amzani · 2024-05-23T08:20:43Z

src/commands/optimize.ts

@@ -40,6 +40,7 @@ export default class Optimize extends Command {
    optimization: Flags.string({char: 'p', default: Object.values(Optimizations), options: Object.values(Optimizations), multiple: true, description: 'select the type of optimizations that you want to apply.'}),
    output: Flags.string({char: 'o', default: Outputs.TERMINAL, options: Object.values(Outputs), description: 'select where you want the output.'}),
    'no-tty': Flags.boolean({ description: 'do not use an interactive terminal', default: false }),
+    disableOptimizationForSchema: Flags.boolean({ description: 'schema should be ignored from the optimization prcoess', default: false }),


Could we have a shorter and DX friendly flag ?
What about: --omit=schema in case we want to disable something else in the future we can reuse the same flag
--omit=components...
cc @KhudaDad414

Yes, even I was thinking that. I just used this since it was suggested here #1432 (comment)

makes sense. In the original issue, I suggested --ignore but --omit also works.

Just for confirmation @Amzani, when you mention other things that can be omitted by changing the flag format, do you mean this, like reuseComponents, removeComponents etc can also be enabled or disabled by default?

{ output: 'YAML', rules: { reuseComponents: true, removeComponents: true, moveAllToComponents: true, moveDuplicatesToComponents: false, }, disableOptimizationFor: { schema: false, }, }

@KhudaDad414 is the best person to answer this one.

During all the time I've been working with Optimizer, I didn't see a need to disable optimization for anything else except schema, and that was only because schemas were disrupting the view of optimized AsyncAPI Document. So I think if at least one user requests anything else, it would be possible to think about changes to the logic, and for now it can be simply the introduction of one consistent across versions switch disableOptimizationForSchema and that's it.

@KhudaDad414 wdyt?

@aeworxet I agree with @helios2003.

I don't think it makes a lot of sense to try to match the command names from optimizer to CLI. they are separate tools and they can evolve separately. purely from the DX point of view having a long command line options like disableOptimizationForSchema isn't ideal since user has to type it every time.
that's why I think having something like --ignore or --omit makes sense here.

optimizer is a library and it is used in other tools, so if we have a long descriptive option names, it doesn't matter since user will only deal with it once in their code.

they are separate tools and they can evolve separately

I don't have a strong opinion in this case either.
@helios2003, please refer to asyncapi/optimizer#252 in which disableOptimizationFor was introduced, to see a list of all places where it was used at once.

@helios2003 @aeworxet tbh I don't have a strong opinion here. one one side

disableOptimizationFor: { schema: false, }

can be used to add more components to ignore in the future. things like:

disableOptimizationFor: { schema: false, message: true }

the question is whether it's a valid case? will someone want this feature? I don't know.

and on the other hand changing the name to disableOptimizationForSchema would make a more seemless way of integration between CLI and optimizer.

I am bit confused. So do I need to add additional components to the flags and change it in both the repositories or just do it for schema?

@helios2003 since optimizer currently only supports schemas I think you can only support schemas here and call it a day.

peter-rr · 2024-05-23T12:10:01Z

src/commands/optimize.ts

@@ -40,6 +40,7 @@ export default class Optimize extends Command {
    optimization: Flags.string({char: 'p', default: Object.values(Optimizations), options: Object.values(Optimizations), multiple: true, description: 'select the type of optimizations that you want to apply.'}),
    output: Flags.string({char: 'o', default: Outputs.TERMINAL, options: Object.values(Outputs), description: 'select where you want the output.'}),
    'no-tty': Flags.boolean({ description: 'do not use an interactive terminal', default: false }),
+    disableOptimizationForSchema: Flags.boolean({ description: 'schema should be ignored from the optimization prcoess', default: false }),


Just a typo here :)

Suggested change

disableOptimizationForSchema: Flags.boolean({ description: 'schema should be ignored from the optimization prcoess', default: false }),

disableOptimizationForSchema: Flags.boolean({ description: 'schema should be ignored from the optimization process', default: false }),

aeworxet · 2024-05-24T12:13:26Z

src/commands/optimize.ts

+    if (flags['disableOptimizationForSchema'] === true) {
+      flags.disableOptimizationForSchema = true;
+    }


You basically wrote

if (flags['disableOptimizationForSchema'] === true) { flags['disableOptimizationForSchema'] = true; }

cuz just to make sure.

I tried set the flag so it can be used in the optimizedDocument.

The very presence of a command line switch in this CLI already means 'true' and its absence - 'false', so you already imported the value of this cmd switch into the JS execution context in lines R106-R108.
It's done in Bundler the same way:
https://github.com/asyncapi/cli/blob/master/src/commands/bundle.ts#L44

This makes code in lines R85-R87 redudant and it can be deleted.

Oh yes, my bad. I don't know how I missed it. Thank you. I will incorporate all the necessary changes in the future commit to this PR.

sonarcloud · 2024-06-05T04:43:36Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

helios2003 · 2024-06-17T07:41:32Z

Hey folks, I apologize for the delay as I've been busy over the past week or two. I will address all the comments made by the maintainers on this PR in a few days. Thank you all for your patience and cooperation.

sonarcloud · 2024-06-20T08:13:27Z

Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

helios2003 · 2024-06-24T04:03:08Z

I have made the changes that were talked about in the discussion. The PR can be reviewed.

KhudaDad414

Hey @helios2003,
I tested the result and it works great. there is one issue that I think should be solved before we can merge this one.

If I pass the ignore flag in the getOptimizedDocument stage, then the report won't reflect it. I mean a component will appear as it's being removed but it will not be removed.

I think this flag should be passed at the (getReport())[https://github.com/asyncapi/optimizer/blob/4fe33fe499d3b1add61f70a27d720f5c8d8e248c/src/Optimizer.ts#L62] stage. but the problem with this approach would be that the user won't see the report to decide if they want to ignore the schemas or not.

I think the best approach would be to accept the flag but also have the option to change it later on with this screen:

this way, if someone is using --no-tty then they will not sure the Report and it doesn't matter if the report doesn't reflect the ignored schema.
And if someone is going the standard way, then they have an option to change their mind about ignoring schemas mid-way. (or ignore them after seeing the report)

helios2003 · 2024-07-04T19:30:35Z

I have tried making the changes as asked, but I am not sure if this is the correct way to implement it. PTAL @KhudaDad414

KhudaDad414

Suggested some changes.

KhudaDad414 · 2024-07-05T08:25:43Z

src/commands/optimize.ts

@@ -178,6 +190,11 @@ export default class Optimize extends Command {
      this.showOptimizations(report.reuseComponents);
      choices.push({name: 'reuse components', value: Optimizations.REUSE_COMPONENTS});
    }
+    if (canIgnoreSchema) {
+      this.log("Do not ignore schema for the components");


strings must use single quote.

KhudaDad414 · 2024-07-05T08:29:07Z

src/commands/optimize.ts

@@ -152,6 +163,7 @@ export default class Optimize extends Command {
    const canMoveAll = report.moveAllToComponents?.length;
    const canRemove = report.removeComponents?.length;
    const canReuse = report.reuseComponents?.length;
+    const canIgnoreSchema = this.disableOptimizations?.includes(DisableOptimizations.SCHEMA);


I don't think we need this. we always show this option.

So, we allow the user to ignore the schema halfway through even if they didn't put the flag initially and vice versa?

Yep, that's the idea.

I included these changes in the last commit.

KhudaDad414 · 2024-07-05T08:35:11Z

src/commands/optimize.ts

@@ -178,6 +190,11 @@ export default class Optimize extends Command {
      this.showOptimizations(report.reuseComponents);
      choices.push({name: 'reuse components', value: Optimizations.REUSE_COMPONENTS});
    }
+    if (canIgnoreSchema) {
+      this.log("Do not ignore schema for the components");
+      choices.push({name: 'Do not ignore schema', value: DisableOptimizations.SCHEMA});


I suggest we use the same option as flag. I mean the name should be Ignore schemas. Also it should be checked by default if --ignore=schema is passed.

helios2003 · 2024-07-29T07:21:01Z

Hi maintainers, can you please review the PR? It has been pending for quite a while.
cc: @Amzani, @KhudaDad414, @Souvikns, @Shurtu-gal

KhudaDad414

LGTM!

helios2003 · 2024-07-30T09:43:40Z

/rtm

Amzani

LGTM

Shurtu-gal · 2024-07-31T05:17:40Z

/rtm

Shurtu-gal · 2024-07-31T05:23:31Z

/rtm

Shurtu-gal · 2024-07-31T05:35:18Z

/u

sonarcloud · 2024-07-31T06:22:11Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

asyncapi-bot · 2024-07-31T07:16:09Z

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

helios2003 requested review from Souvikns, Amzani, Shurtu-gal and asyncapi-bot-eve as code owners May 22, 2024 18:49

Amzani requested changes May 23, 2024

View reviewed changes

peter-rr reviewed May 23, 2024

View reviewed changes

aeworxet reviewed May 24, 2024

View reviewed changes

fixes in ignore flag

de2dd43

helios2003 force-pushed the optimize branch from 68e3a21 to de2dd43 Compare June 24, 2024 03:55

KhudaDad414 suggested changes Jul 1, 2024

View reviewed changes

add flexibility to the ignore flag

57df499

KhudaDad414 suggested changes Jul 5, 2024

View reviewed changes

give option for schema everytime

3558b42

helios2003 requested a review from KhudaDad414 July 10, 2024 11:35

KhudaDad414 approved these changes Jul 30, 2024

View reviewed changes

Merge branch 'master' into optimize

f975095

asyncapi-bot added the ready-to-merge label Jul 30, 2024

Amzani approved these changes Jul 30, 2024

View reviewed changes

Merge branch 'master' into optimize

1d073f7

asyncapi-bot removed the ready-to-merge label Jul 31, 2024

asyncapi-bot and others added 2 commits July 31, 2024 07:36

Merge branch 'master' into optimize

a0e2788

Merge branch 'master' into optimize

bf3f5f1

Shurtu-gal merged commit b825a34 into asyncapi:master Jul 31, 2024
10 checks passed

asyncapi-bot added the released label Jul 31, 2024

	disableOptimizationForSchema: Flags.boolean({ description: 'schema should be ignored from the optimization prcoess', default: false }),
	disableOptimizationForSchema: Flags.boolean({ description: 'schema should be ignored from the optimization process', default: false }),

feat: add the flag for disabling optimizations #1447

feat: add the flag for disabling optimizations #1447

Conversation

helios2003 commented May 22, 2024

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

KhudaDad414 Jun 3, 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

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

sonarcloud bot commented Jun 5, 2024

Quality Gate passed

helios2003 commented Jun 17, 2024

sonarcloud bot commented Jun 20, 2024

Quality Gate failed

helios2003 commented Jun 24, 2024

KhudaDad414 left a comment

Choose a reason for hiding this comment

helios2003 commented Jul 4, 2024

KhudaDad414 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helios2003 Jul 5, 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

helios2003 commented Jul 29, 2024

KhudaDad414 left a comment

Choose a reason for hiding this comment

helios2003 commented Jul 30, 2024

Amzani left a comment

Choose a reason for hiding this comment

Shurtu-gal commented Jul 31, 2024

Shurtu-gal commented Jul 31, 2024

Shurtu-gal commented Jul 31, 2024

sonarcloud bot commented Jul 31, 2024

Quality Gate passed

asyncapi-bot commented Jul 31, 2024

KhudaDad414 Jun 3, 2024 •

edited

Loading

helios2003 Jul 5, 2024 •

edited

Loading