feat: integrate new ParserJS with Spectral #434

magicmatatjahu · 2022-09-27T08:28:04Z

Description

Integrate new v2 ParserJS with Spectral support.
Update SpecificationService to handle new parsing behaviour.
Studio still uses in rendering old parser-api - it will be rewritten to new one in the next PRs.
update webpack config to handle internal Spectral deps.
added config for governance in Settings Modal - now we can show/hide warnings/info/hint governance issues.

Example:

Related issue(s)
Part of asyncapi/parser-js#481

netlify · 2022-09-27T08:28:10Z

✅ Deploy Preview for modest-rosalind-098b67 ready!

Name	Link
🔨 Latest commit	`73e6211`
🔍 Latest deploy log	https://app.netlify.com/sites/modest-rosalind-098b67/deploys/637b4f65a8de26000807e5bf
😎 Deploy Preview	https://deploy-preview-434--modest-rosalind-098b67.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

smoya · 2022-10-10T08:47:14Z

Netlify build fails because of asyncapi/parser-js#650

Why our CI didn't fail? 🤔

cc @magicmatatjahu @derberg any idea?

smoya · 2022-10-10T16:34:53Z

@magicmatatjahu can you please bump @asyncapi/parser to 2.0.0-next-major.6 so we fix Netlify build? (I have no permissions to push to your branch unfortunately).

jonaslagoni

Love the new error handling! 🔥 And how accurately it highlights the areas that has problems.

Hate the warning system though 😆 As you are entering the realm of governance rules, which highly vary from user to user. I would HATE to see warnings for an arbitrary rule that someone says I should support, for example, Info object must have "contact" object., as it's not a requirement from the AsyncAPI spec, but spectral.

For some, this will become highly confusing.

I suggest anything beyond requirements from the spec should be turned off, until (if it's being planned) it's possible to personally select linting rules.

And it makes the editor almost unusable with yellow markings all over 😄 Which almost forces you to take action on the warnings.

Also, are warnings supposed to be defined as problems? 🤔

magicmatatjahu · 2022-10-28T12:12:21Z

@jonaslagoni Thanks for quick review!

Hate the warning system though 😆 As you are entering the realm of governance rules, which highly vary from user to user. I would HATE to see warnings for an arbitrary rule that someone says I should support, for example, Info object must have "contact" object., as it's not a requirement from the AsyncAPI spec, but spectral.

Warnings inform you about some good practices and we should enable them by default - it's not treated as error and studio will render html documentation on the right, it's only a info to user that something is a good practice 😄 Of course description of such a warning should be defined as Info object should have "contact" object. not "must" but it's a "bug" on Spectral side not our. I don't wanna wait for fixing such a thing on Spectral side, because we will make integration next 1-2 months and it's not a big bug at all.

I suggest anything beyond requirements from the spec should be turned off, until (if it's being planned) it's possible to personally select linting rules.

Yeah, custom rules and overrides are in plan, but probably it will be enabled in next year.

And it makes the editor almost unusable with yellow markings all over 😄 Which almost forces you to take action on the warnings.

Yeah, I hate it either 😆 so I added suggestion (if you didn't see that) in slack:

The only problem I have with the current integration is that we underline even warnings/hints in the editor and not just errors (which should be underlined), making the document itself in the editor have too many "colors" and difficult to read, even document may be correct. Should we inform about warnings and hints in the left side of the editor (where the line numbers are) with warning and hint icons? What do you think about this.

So we should underline only errors (by red color) and warning and hints should be displayed in the left panel (where lines number are) as icon - example: https://microsoft.github.io/monaco-editor/playground.html#interacting-with-the-editor-rendering-glyphs-in-the-margin check this red square in editor.

Also, are warnings supposed to be defined as problems? 🤔

We use monaco-editor, this same which VSC uses, so "warnings", "hints" etc are also considering as "problems" - VSC treats every error/waring/hint as problem not "diagnostic" as we do and I don't think so that we can change description of such a warnings.

jonaslagoni · 2022-10-28T12:25:56Z

Warnings inform you about some good practices and we should enable them by default

I understand that it makes sense to have good practices highlighted, but the matter of the facts is that its only relevant for some users. And it comes down to their use-case and what they would like to see as good practice.

If you develop an internal application and use AsyncAPI to describe it and you are a small team, adding contact info makes no sense.

^just an example, sure in some cases it might be, but for small teams or solos, it does not and does nothing but create friction 🙂

If you really want it, I suggest it be added as part of the settings so it can be turned off at the will of the user.

Of course description of such a warning should be defined as Info object should have "contact" object. not "must" but it's a "bug" on Spectral side not our. I don't wanna wait for fixing such a thing on Spectral side, because we will make integration next 1-2 months and it's not a big bug at all.

That's understandable 🙂

So we should underline only errors (by red color) and warning and hints should be displayed in the left panel (where lines number are) as icon - example: https://microsoft.github.io/monaco-editor/playground.html#interacting-with-the-editor-rendering-glyphs-in-the-margin check this red square in editor.

Hard to say 😆 Cause if you then have to add one for each line where the yellow is currently highlighted, it's also quite a lot still 😄

magicmatatjahu · 2022-10-28T12:33:32Z

I understand that it makes sense to have good practices highlighted, but the matter of the facts is that its only relevant for some users. And it comes down to their use-case and what they would like to see as good practice.

If you really want it, I suggest it be added as part of the settings so it can be turned off at the will of the user.

I understand but I don't like such "filtering", because warning contact is indeed unnecessary but for operationId that should define is good imho 😄 I personally would leave it, for me it's no problem, but I can add options to enable/disable these warnings. I will make it, but probably not today 😄

Hard to say 😆 Cause if you then have to add one for each line where the yellow is currently highlighted, it's also quite a lot still 😄

That icon will be only visible in starting line, so if your object is defined in 20 lines you will only see that icon on first line.

jonaslagoni · 2022-10-28T12:38:39Z

I understand but I don't like such "filtering", because warning contact is indeed unnecessary but for operationId that should define is good imho 😄 I personally would leave it, for me it's no problem, but I can add options to enable/disable these warnings. I will make it, but probably not today 😄

Just a global one for all warnings I think should be enough, for example, disable good practice warnings or something like that.

That icon will be only visible in starting line, so if your object is defined in 20 lines you will only see that icon on first line.

That would probably make sense then 👍

magicmatatjahu · 2022-11-02T12:11:17Z

I added vertical lines in the left side of editor (where lines are):

Errors are still visible as markers:

cc @smoya @jonaslagoni what do you think?

derberg · 2022-11-08T12:09:37Z

I would never guess I can modify the diagnostic settings. Imho there should be some icon/link that I can click, and it will open up a setting for me, with a focus on the proper tab.

src/components/Terminal/ProblemsTab.tsx

magicmatatjahu · 2022-11-08T12:11:04Z

@derberg Ok I will do that, thanks for suggestion!

magicmatatjahu · 2022-11-08T14:13:34Z

@derberg Done :) We have gear icon in the diagnostics tab.

src/components/Modals/Settings/SettingsModal.tsx

fmvilas · 2022-11-08T15:28:40Z

src/components/Terminal/ProblemsTab.tsx

+  case 1: return (
+    <div className='flex flex-row items-center justify-center'>
+      <VscWarning className='text-yellow-500 w-4 h-4' />
+    </div>
+  );
+  case 2: return (
+    <div className='flex flex-row items-center justify-center'>
+      <VscInfo className='text-blue-500 w-4 h-4' />
+    </div>
+  );
+  case 3: return (
+    <div className='flex flex-row items-center justify-center'>
+      <VscLightbulb className='text-green-500 w-4 h-4' />
+    </div>
+  );
+  default: return (
+    <div className='flex flex-row items-center justify-center'>
+      <VscError className='text-red-500 w-4 h-4' />
    </div>


It would be awesome to have tooltips in these icons. Until I came to the code, I couldn't figure out what the green light bulb meant.

I hope that you like it :)

magicmatatjahu · 2022-11-09T10:09:23Z

@fmvilas I applied all suggestions. Could you check again? Thanks!
@derberg You added suggestion to have button in tab to open the Governance settings. Could you check and let me know if you like it. I'll be very grateful :)

derberg · 2022-11-09T14:56:43Z

works like a charm ❤️

fmvilas

🐑 🇮🇹

magicmatatjahu · 2022-11-14T09:29:28Z

@fmvilas Conflicts are resolved. Could you accept again?

fmvilas

Getting better but still got UX issues.

fmvilas · 2022-11-15T13:31:00Z

src/components/Terminal/ProblemsTab.tsx

+
+  return (
+    <ul className='flex flex-row items-center'>
+      <Tooltip content="Show only errors">


The tooltip is great if this option is not selected but when I click on the button it still says "Show only errors", and it's wrong. It should probably say "Show everything" or "Stop showing only errors" or something like that.

That said, playing with these buttons I realized it's acting as a "select" or "option". Wouldn't it make more sense to accept selecting multiple options? For instance, I want to see errors and warnings only. This is a common thing when you're fixing problems and don't want to have much noise. Also, tooltips will become easier. They'd be something like "Show errors" or "Hide errors".

Ok, I implemented this second approach with Show all diagnostics text if user select only one option :) Play with it and you will see what I have in mind. Thanks for suggestion!

The default state doesn't make sense. It says "Show warnings" but I'm already seeing warnings 😅 I think, by default, all buttons should be checked.

I think, by default, all buttons should be checked.

It doesn't have a sense, because then people will need to uncheck three buttons if they will want to see only warnings. I changed state to show messages like:

buttons are unchecked: Show only {diagnostic type}

one button is checked and we hover this checked button: Show all diagnostics and for other Show {diagnostic type}

for rest: Show {diagnostic type} or Hide {diagnostic type}

WDYT? :) You can check in preview :)

Ok. Fair. It's weird that all buttons checked and all buttons unchecked will show you the same thing 😅

One more thing. I clicked on the Settings cog icon:

And it lets me disable warning, information, and hint messages. In such case, I think we should disable the buttons in this menu too and when you hover over them you'll see a tooltip saying it's been disabled in the settings and that you have to enable them back if you want to use them.

Ok. Fair. It's weird that all buttons checked and all buttons unchecked will show you the same thing 😅

We should leave it as it's now, because people should have option to disable only one particular kind of diagnostics, when you have enabled others. The current solution isn't perfect but by changing it, people may have a worse UX.

And it lets me disable warning, information, and hint messages. In such case, I think we should disable the buttons in this menu too and when you hover over them you'll see a tooltip saying it's been disabled in the settings and that you have to enable them back if you want to use them.

I will add.

Alright, this is looking much better now 👏

src/components/Terminal/ProblemsTab.tsx

magicmatatjahu · 2022-11-15T15:56:40Z

@fmvilas Thanks for review! Could you check again?

src/components/Terminal/ProblemsTab.tsx

sonarcloud · 2022-11-21T10:15:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

magicmatatjahu · 2022-11-21T11:44:30Z

@fmvilas Could you make review again?

magicmatatjahu · 2022-11-21T14:32:49Z

Finally 🚀 Thanks for everyone for review!

magicmatatjahu · 2022-11-21T14:32:55Z

/rtm

asyncapi-bot · 2022-11-21T14:52:19Z

🎉 This PR is included in version 0.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

magicmatatjahu added the enhancement New feature or request label Sep 27, 2022

magicmatatjahu mentioned this pull request Sep 27, 2022

Road to v2.0.0 asyncapi/parser-js#481

Closed

20 tasks

magicmatatjahu force-pushed the spectral-integration branch from ccaa62e to 1371fa5 Compare October 5, 2022 10:42

smoya mentioned this pull request Oct 10, 2022

fix: set lockfile version to 2 fixing issues with @asyncapi/parser de… asyncapi/parser-js#650

Merged

magicmatatjahu mentioned this pull request Oct 25, 2022

Custom schema format produces an error #460

Closed

magicmatatjahu force-pushed the spectral-integration branch 2 times, most recently from c36e682 to 7d64025 Compare October 28, 2022 11:19

magicmatatjahu marked this pull request as ready for review October 28, 2022 11:22

magicmatatjahu requested review from fmvilas, boyney123, mcturco and asyncapi-bot-eve as code owners October 28, 2022 11:22

jonaslagoni suggested changes Oct 28, 2022

View reviewed changes

magicmatatjahu mentioned this pull request Nov 8, 2022

Dereference error does not include "title" asyncapi/parser-js#561

Closed

derberg reviewed Nov 8, 2022

View reviewed changes

src/components/Terminal/ProblemsTab.tsx Outdated Show resolved Hide resolved

fmvilas requested changes Nov 8, 2022

View reviewed changes

magicmatatjahu requested review from fmvilas and derberg November 9, 2022 10:19

fmvilas previously approved these changes Nov 14, 2022

View reviewed changes

magicmatatjahu mentioned this pull request Nov 14, 2022

fix: update @asyncapi dependencies #483

Merged

magicmatatjahu dismissed fmvilas’s stale review via 6193248 November 14, 2022 08:20

magicmatatjahu force-pushed the spectral-integration branch from e034612 to 6193248 Compare November 14, 2022 08:20

magicmatatjahu force-pushed the spectral-integration branch from 3741ed9 to f72b744 Compare November 15, 2022 11:53

magicmatatjahu requested a review from fmvilas November 15, 2022 12:20

fmvilas requested changes Nov 15, 2022

View reviewed changes

magicmatatjahu requested a review from fmvilas November 15, 2022 15:56

fmvilas reviewed Nov 19, 2022

View reviewed changes

src/components/Terminal/ProblemsTab.tsx Outdated Show resolved Hide resolved

magicmatatjahu force-pushed the spectral-integration branch from 0e6ecbb to 16d0d01 Compare November 21, 2022 09:22

magicmatatjahu added 2 commits November 21, 2022 10:24

feat: integrate new ParserJS with Spectral

853b2ef

add integration

acc8a2d

magicmatatjahu force-pushed the spectral-integration branch from 16d0d01 to acc8a2d Compare November 21, 2022 09:24

apply review suggestions

73e6211

magicmatatjahu requested a review from fmvilas November 21, 2022 11:44

fmvilas approved these changes Nov 21, 2022

View reviewed changes

asyncapi-bot added the ready-to-merge label Nov 21, 2022

asyncapi-bot merged commit 0a6d9fb into asyncapi:master Nov 21, 2022

magicmatatjahu deleted the spectral-integration branch November 21, 2022 14:33

asyncapi-bot added the released label Nov 21, 2022

magicmatatjahu mentioned this pull request Nov 21, 2022

feat: added tooltips for navigation bar (#193) #215

Closed

KhudaDad414 pushed a commit to KhudaDad414/studio that referenced this pull request Oct 12, 2023

chore(release): v0.21.0 (asyncapi#434)

a00a1e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate new ParserJS with Spectral #434

feat: integrate new ParserJS with Spectral #434

magicmatatjahu commented Sep 27, 2022 •

edited

Loading

netlify bot commented Sep 27, 2022 •

edited

Loading

smoya commented Oct 10, 2022 •

edited

Loading

smoya commented Oct 10, 2022

jonaslagoni left a comment

magicmatatjahu commented Oct 28, 2022 •

edited

Loading

jonaslagoni commented Oct 28, 2022 •

edited

Loading

magicmatatjahu commented Oct 28, 2022 •

edited

Loading

jonaslagoni commented Oct 28, 2022

magicmatatjahu commented Nov 2, 2022

derberg commented Nov 8, 2022

magicmatatjahu commented Nov 8, 2022

magicmatatjahu commented Nov 8, 2022

fmvilas Nov 8, 2022

magicmatatjahu Nov 9, 2022

magicmatatjahu commented Nov 9, 2022

derberg commented Nov 9, 2022

fmvilas left a comment

magicmatatjahu commented Nov 14, 2022

fmvilas left a comment

fmvilas Nov 15, 2022

magicmatatjahu Nov 15, 2022

fmvilas Nov 15, 2022

magicmatatjahu Nov 16, 2022 •

edited

Loading

fmvilas Nov 18, 2022

fmvilas Nov 18, 2022

magicmatatjahu Nov 21, 2022

magicmatatjahu Nov 21, 2022

fmvilas Nov 21, 2022

magicmatatjahu commented Nov 15, 2022

sonarcloud bot commented Nov 21, 2022

magicmatatjahu commented Nov 21, 2022

magicmatatjahu commented Nov 21, 2022

magicmatatjahu commented Nov 21, 2022

asyncapi-bot commented Nov 21, 2022

feat: integrate new ParserJS with Spectral #434

feat: integrate new ParserJS with Spectral #434

Conversation

magicmatatjahu commented Sep 27, 2022 • edited Loading

netlify bot commented Sep 27, 2022 • edited Loading

✅ Deploy Preview for modest-rosalind-098b67 ready!

smoya commented Oct 10, 2022 • edited Loading

smoya commented Oct 10, 2022

jonaslagoni left a comment

Choose a reason for hiding this comment

magicmatatjahu commented Oct 28, 2022 • edited Loading

jonaslagoni commented Oct 28, 2022 • edited Loading

magicmatatjahu commented Oct 28, 2022 • edited Loading

jonaslagoni commented Oct 28, 2022

magicmatatjahu commented Nov 2, 2022

derberg commented Nov 8, 2022

magicmatatjahu commented Nov 8, 2022

magicmatatjahu commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicmatatjahu commented Nov 9, 2022

derberg commented Nov 9, 2022

fmvilas left a comment

Choose a reason for hiding this comment

magicmatatjahu commented Nov 14, 2022

fmvilas 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

magicmatatjahu Nov 16, 2022 • 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

magicmatatjahu commented Nov 15, 2022

sonarcloud bot commented Nov 21, 2022

magicmatatjahu commented Nov 21, 2022

magicmatatjahu commented Nov 21, 2022

magicmatatjahu commented Nov 21, 2022

asyncapi-bot commented Nov 21, 2022

magicmatatjahu commented Sep 27, 2022 •

edited

Loading

netlify bot commented Sep 27, 2022 •

edited

Loading

smoya commented Oct 10, 2022 •

edited

Loading

magicmatatjahu commented Oct 28, 2022 •

edited

Loading

jonaslagoni commented Oct 28, 2022 •

edited

Loading

magicmatatjahu commented Oct 28, 2022 •

edited

Loading

magicmatatjahu Nov 16, 2022 •

edited

Loading