Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add global specification watcher #220

Merged
merged 32 commits into from
Feb 21, 2022
Merged

feat: add global specification watcher #220

merged 32 commits into from
Feb 21, 2022

Conversation

imabp
Copy link
Member

@imabp imabp commented Feb 8, 2022 •
edited
Loading

Description

  • Adds Global Watcher for Specification

image

Related issue(s)
#73

@imabp imabp mentioned this pull request Feb 8, 2022
Closed
@imabp
Copy link
Member Author

imabp commented Feb 9, 2022

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 requested changes Feb 9, 2022
View reviewed changes
Copy link
Member

@derberg derberg left a comment

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?

src/globals.ts Outdated Show resolved Hide resolved
src/globals.ts Outdated Show resolved Hide resolved
src/commands/validate.ts Outdated Show resolved Hide resolved
src/commands/validate.ts Outdated Show resolved Hide resolved
@imabp
Copy link
Member Author

imabp commented Feb 9, 2022

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

@imabp imabp requested a review from derberg February 9, 2022 09:35
magicmatatjahu
magicmatatjahu suggested changes Feb 9, 2022
View reviewed changes
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

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.

src/commands/validate.ts Outdated Show resolved Hide resolved
src/globals.ts Outdated Show resolved Hide resolved
src/globals.ts Outdated Show resolved Hide resolved
magicmatatjahu
magicmatatjahu suggested changes Feb 9, 2022
View reviewed changes
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

@imabp Good, but you have to handle that one code smell :) https://sonarcloud.io/project/issues?id=asyncapi_cli&open=AX7bDmqtPXvNfWF5cO0t&pullRequest=220&resolved=false&types=CODE_SMELL

Also please refer to my and Łukasz suggestion: https://github.com/asyncapi/cli/pull/220/files#r802512653

@imabp
Copy link
Member Author

imabp commented Feb 9, 2022

Hey @magicmatatjahu the code smell is fixed :)

derberg
derberg requested changes Feb 10, 2022
View reviewed changes
Copy link
Member

@derberg derberg left a comment

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

src/commands/diff.ts Outdated Show resolved Hide resolved
src/flags.ts Show resolved Hide resolved
@imabp
Copy link
Member Author

imabp commented Feb 10, 2022

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

image

derberg
derberg requested changes Feb 10, 2022
View reviewed changes
Copy link
Member

@derberg derberg left a comment

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

src/globals.ts Outdated Show resolved Hide resolved
src/globals.ts Outdated Show resolved Hide resolved
src/globals.ts Outdated Show resolved Hide resolved
magicmatatjahu
magicmatatjahu reviewed Feb 10, 2022
View reviewed changes
src/globals.ts Outdated Show resolved Hide resolved
src/flags.ts Show resolved Hide resolved
@imabp @derberg
fix: change log color
15d58ca 
Co-authored-by: Lukasz Gornicki <[email protected]>
@imabp imabp mentioned this pull request Feb 18, 2022
Closed
@imabp
Copy link
Member Author

imabp commented Feb 18, 2022

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

@imabp
Copy link
Member Author

imabp commented Feb 18, 2022

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
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
Copy link
Member Author

imabp commented Feb 18, 2022

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
Member

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

image

@imabp
Copy link
Member Author

imabp commented Feb 18, 2022

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
Copy link
Member Author

imabp commented Feb 18, 2022

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

magicmatatjahu
magicmatatjahu reviewed Feb 18, 2022
View reviewed changes
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Last thing :)

src/commands/diff.ts Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Feb 21, 2022

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 approved these changes Feb 21, 2022
View reviewed changes
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

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
Member

/rtm

@magicmatatjahu magicmatatjahu merged commit 1021b6b into asyncapi:master Feb 21, 2022
@imabp imabp mentioned this pull request Feb 21, 2022
Closed
@imabp
Copy link
Member Author

imabp commented Feb 21, 2022 •
edited
Loading

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
Contributor

🎉 This PR is included in version 0.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@imabp
Copy link
Member Author

imabp commented Feb 21, 2022

@all-contributors please add @imabp for code

@allcontributors allcontributors bot mentioned this pull request Feb 21, 2022
Merged
@allcontributors
Copy link
Contributor

@imabp

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

@imabp
Copy link
Member Author

imabp commented Feb 21, 2022

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

@imabp imabp mentioned this pull request Mar 5, 2022
Closed
16 tasks
Souvikns added a commit to Souvikns/cli that referenced this pull request Mar 8, 2022
@Souvikns @imabp
@derberg @magicmatatjahu @asyncapi-bot @allcontributors @Samridhi-98
Prepack fix (#1)
994e294 
* docs: fix missing toc (asyncapi#230)

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

Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Maciej Urbańczyk <[email protected]>

* 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 <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Maciej Urbańczyk <[email protected]>
Co-authored-by: asyncapi-bot <[email protected]>
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: Samriddhi <[email protected]>
Co-authored-by: asyncapi-bot <[email protected]>
Souvikns added a commit to Souvikns/cli that referenced this pull request Mar 8, 2022
@Souvikns @imabp
@derberg @magicmatatjahu @asyncapi-bot @allcontributors @Samridhi-98
Prepack fix (#2)
bfc3505 
* docs: fix missing toc (asyncapi#230)

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

Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Maciej Urbańczyk <[email protected]>

* 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

* Update test.yml

Co-authored-by: Abir <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Maciej Urbańczyk <[email protected]>
Co-authored-by: asyncapi-bot <[email protected]>
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: Samriddhi <[email protected]>
Co-authored-by: asyncapi-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magicmatatjahu magicmatatjahu magicmatatjahu approved these changes

@derberg derberg Awaiting requested review from derberg

Assignees
No one assigned
Labels
do-not-merge ready-to-merge released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@imabp @magicmatatjahu @asyncapi-bot @derberg