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.

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

feat: created custom help message #126

Merged
merged 55 commits into from
Dec 7, 2021
Merged

feat: created custom help message #126

merged 55 commits into from
Dec 7, 2021

Conversation

Souvikns
Copy link
Member

@Souvikns Souvikns commented Nov 3, 2021

Description

Add the reverted changes from #124

Souvikns added 30 commits June 18, 2021 18:16
This is was causing the test cases to fail.
fmvilas
fmvilas previously approved these changes Nov 29, 2021
@derberg
Copy link
Member

derberg commented Nov 29, 2021

@Souvikns just curious why we need to ignore so many files in the case of Sonar Cloud scans?

@Souvikns
Copy link
Member Author

It was showing some function is bigger than it should be, but in this case, it is needed, and in the command directory there were some warnings for the function naming conventions, which were needed by oclif to work correctly,

@derberg
Copy link
Member

derberg commented Nov 30, 2021

@Souvikns but we are ignoring not specific file but all src/help/** with wildcard.

  • so for the cognitive complexity instead of ignoring in config I suggest you just put /* eslint-disable sonarjs/cognitive-complexity */ before the complex function that can't be simpler
  • in case of naming conflict, just ignore specific files and add comments there why there are ignored so in the future it is clear

@magicmatatjahu
Copy link
Member

@Souvikns Also, if you need to disable sonar (not eslint, but exactly sonar cloud) only for certain function, you can add comment // NOSONAR next to function declaration/type etc:

function fn() { // NOSONAR

@Souvikns
Copy link
Member Author

Souvikns commented Dec 1, 2021

I will be removing the sonar.exclusion property and see what errors I am getting and then I will solve them one by one

@Souvikns
Copy link
Member Author

Souvikns commented Dec 1, 2021

@derberg @magicmatatjahu I have removed all .sonarcloud.properties file as there was only exclusion, now sonar cloud is scanning the complete codebase, and I am resolved the code smells.

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.

@Souvikns there are still linter errors about cognitive complexity. Ignore them the way I wrote in my previous comment.

we need "lint": "eslint --max-warnings 5 --config .eslintrc .", to change to "lint": "eslint --max-warnings 0 --config .eslintrc .",

@Souvikns
Copy link
Member Author

Souvikns commented Dec 2, 2021

I have removed the warnings but for some reason, I am getting new code smells, Will be fixing them soon.

@Souvikns
Copy link
Member Author

Souvikns commented Dec 2, 2021

I guess I need @magicmatatjahu and @jotamusik approval to merge

@derberg
Copy link
Member

derberg commented Dec 2, 2021

@Souvikns at least @magicmatatjahu as he "requested changes" initially and we can't merge before he accepts

@Souvikns
Copy link
Member Author

Souvikns commented Dec 2, 2021

Yeah, I made the changes @magicmatatjahu requested, I have removed the help command and now the only --help flag works, so now you won't see an extra help command in the root help.

Let me show how the help message is looking right now.

Root Help
image

Command Help
image

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! :)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 6, 2021

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

@Souvikns
Copy link
Member Author

Souvikns commented Dec 6, 2021

@jotamusik waiting from your review

@derberg
Copy link
Member

derberg commented Dec 6, 2021

@Souvikns just fyi you do not need a review of every CODEOWNER, this is needed only when we want to add someone to CODEOWNER file. Imho if you have the approval of 2 maintainers here, you are good to go, unless you synced with Jorge and he needs to have a look.

btw tests are failing, please have a look

@Souvikns
Copy link
Member Author

Souvikns commented Dec 6, 2021

ok @derberg I will merge this after I fix the test, They were working before, but when I updated the branch they are failing.

@Souvikns Souvikns merged commit 058114c into asyncapi:master Dec 7, 2021
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants