Skip to content

feat: add global specification watcher#220

Merged
magicmatatjahu merged 32 commits into
asyncapi:masterfrom
imabp:imabp-watch
Feb 21, 2022
Merged

feat: add global specification watcher#220
magicmatatjahu merged 32 commits into
asyncapi:masterfrom
imabp:imabp-watch

Conversation

@imabp

@imabp imabp commented Feb 8, 2022

Copy link
Copy Markdown
Member

Description

  • Adds Global Watcher for Specification

image

Related issue(s)
#73

@imabp imabp mentioned this pull request Feb 8, 2022
@imabp

imabp commented Feb 9, 2022

Copy link
Copy Markdown
Member Author

I guess there is an error with, test case for watch mode.
The fs stream is not getting closed. I will try to create a mock stream and then close it from tests
Apologies, the test ran for 4 hours.

@derberg derberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also enable watcher for diff

@Souvikns @boyney123 @magicmatatjahu wanna have a look?

Comment thread src/globals.ts Outdated
Comment thread src/globals.ts Outdated
Comment thread src/commands/validate.ts Outdated
Comment thread src/commands/validate.ts Outdated
imabp and others added 4 commits February 9, 2022 13:52
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@imabp

imabp commented Feb 9, 2022

Copy link
Copy Markdown
Member Author

Hey @derberg watch flag is added to diff :)
Kindly review :)

@imabp imabp requested a review from derberg February 9, 2022 09:35

@magicmatatjahu magicmatatjahu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I leave some comments, please refer to them.

Comment thread src/commands/validate.ts Outdated
Comment thread src/globals.ts Outdated
Comment thread src/globals.ts Outdated

@magicmatatjahu magicmatatjahu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imabp

imabp commented Feb 9, 2022

Copy link
Copy Markdown
Member Author

Hey @magicmatatjahu the code smell is fixed :)

@derberg derberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH man, what an improvement, it looks much cleaner now. I left only 2 comments

Comment thread src/commands/diff.ts Outdated
Comment thread src/flags.ts
@imabp

imabp commented Feb 10, 2022

Copy link
Copy Markdown
Member Author

Hey @derberg @magicmatatjahu
Please check the latest changes, of auto-disabling watch mode. Here is a screenshot

image

@derberg derberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, except for one followup comments I guess we are still left with an idea on testing watcher?

I'm also wondering if we do not need unit tests for commands where watcher is used. Like what happens for example when user passes --watch and both AsyncAPI references are URLs. Do we perform validation/diff> IMHO yes, but then we need to have it tested imho as well

Comment thread src/globals.ts Outdated
Comment thread src/globals.ts Outdated
Comment thread src/globals.ts Outdated
Comment thread src/globals.ts Outdated
Comment thread src/flags.ts
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@imabp

imabp commented Feb 18, 2022

Copy link
Copy Markdown
Member Author

Sure @magicmatatjahu here is the new issue, we can work on that. :)
#234

@imabp

imabp commented Feb 18, 2022

Copy link
Copy Markdown
Member Author

Just a question, @magicmatatjahu then shall we proceed with this PR, and then address the issue of handling errors #234 ?
Or
Work on #234 and let this PR be on hold. ?

@magicmatatjahu

Copy link
Copy Markdown
Member

@imabp First complete that PR and then start working on mentioned issue :) You can propagate changes from that comment and I will accept it. #220 (comment)

@imabp

imabp commented Feb 18, 2022

Copy link
Copy Markdown
Member Author

I have fixed the second error as you mentioned @magicmatatjahu and is now reflected in the latest changes

another problem: when I provide some changes in the watched file I always see Watching AsyncAPI file at ./sample.yaml on every change. We should remove 40 line from specWatcher function:

image

Here are the checks
image

@magicmatatjahu

Copy link
Copy Markdown
Member

@imabp Probably we don't understand each other 😆 I had in mind this one:

image

@imabp

imabp commented Feb 18, 2022

Copy link
Copy Markdown
Member Author

Ohh I got it.. 😅 . .Its really a learning to converse on PRs 😄 specially on such highly-critical issues
Implementing inside the catch(err) block, sure I will go through that..

@imabp

imabp commented Feb 18, 2022

Copy link
Copy Markdown
Member Author

Hey @magicmatatjahu
Everything seems to work well now 😄
Checked on node LTS
image

@imabp imabp requested a review from magicmatatjahu February 18, 2022 14:20

@magicmatatjahu magicmatatjahu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last thing :)

Comment thread src/commands/diff.ts Outdated
@imabp imabp requested a review from magicmatatjahu February 21, 2022 06:10
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@magicmatatjahu magicmatatjahu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! However I see little "bug" when I run diff command. When I have invalid spec I cannot recognize in which file I have errors (in new or old?):

image

We should improve logs in the followup PR @imabp Could you handle that? We need that PR for another commands so I don't want to block development with rejection.

@magicmatatjahu

Copy link
Copy Markdown
Member

/rtm

@imabp

imabp commented Feb 21, 2022

Copy link
Copy Markdown
Member Author

Yes sure!! @magicmatatjahu We need a proper logging control center 😅 or an interface

I am creating a issue #236
We can work on this after fixing #234

@asyncapi-bot

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 0.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@imabp

imabp commented Feb 21, 2022

Copy link
Copy Markdown
Member Author

@all-contributors please add @imabp for code

@allcontributors

Copy link
Copy Markdown
Contributor

@imabp

I've put up a pull request to add @imabp! 🎉

@imabp

imabp commented Feb 21, 2022

Copy link
Copy Markdown
Member Author

I gave it a try, and it worked out xD 😂😂

@imabp imabp mentioned this pull request Mar 5, 2022
16 tasks
Souvikns added a commit to Souvikns/cli that referenced this pull request Mar 8, 2022
* docs: fix missing toc (asyncapi#230)

* feat: add global specification watcher (asyncapi#220)

Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>

* chore(release): v0.14.0 (asyncapi#237)

* docs: add imabp as a contributor for code (asyncapi#239)

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

* fix: update @asyncapi/studio to 0.10.0 version (asyncapi#241)

* chore(release): v0.14.1 (asyncapi#242)

* feat: added testing for new (asyncapi#212)

* docs: add Samridhi-98 as a contributor for test (asyncapi#248)

* ci: update global workflows (asyncapi#246)

* chore(release): v0.15.0 (asyncapi#249)

* feat: migrate to latest oclif version (asyncapi#203)

* chore: update bot name from codeowners file (asyncapi#247)

* ci: update global workflows (asyncapi#250)

* Create test.yml

* Update test.yml

* Update test.yml

* Update test.yml

* Update test.yml

Co-authored-by: Abir <abir.pal899@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
Co-authored-by: asyncapi-bot <61865014+asyncapi-bot@users.noreply.github.com>
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: Samriddhi <Agrawalsamriddhi83@gmail.com>
Co-authored-by: asyncapi-bot <bot+chan@asyncapi.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants