Add possibility to collect metrics from branch or pullRequest analysis #31
Conversation
src/helpers/file.helpers.ts
Outdated
| export function isPullRequestOrBranchConfigured(config: Config) { | ||
| let returnVal = {}; | ||
|
|
||
| let isBranchConfigured = has(config, 'branch') && !config.branch.includes('optional-branch-name'); |
There was a problem hiding this comment.
replace let by const please
src/helpers/file.helpers.ts
Outdated
| let returnVal = {}; | ||
|
|
||
| let isBranchConfigured = has(config, 'branch') && !config.branch.includes('optional-branch-name'); | ||
| let isPullRequestConfigured = has(config, 'pullRequest') && !config.pullRequest.includes('optional-pull-request-id'); |
src/helpers/file.helpers.ts
Outdated
|
|
||
| if ( isBranchConfigured || isPullRequestConfigured) { | ||
| if ( isBranchConfigured ) { | ||
| returnVal = { branch: config.branch }; |
There was a problem hiding this comment.
why not return directly?
There was a problem hiding this comment.
My apologies. I'm working in another project in C and we use only one return at the end of function. Updated
There was a problem hiding this comment.
you don't have to apologise, you did a great job, my remarks are only improvements nothing else 🎉
src/helpers/file.helpers.ts
Outdated
|
|
||
| // Give pullRequest highest priority | ||
| if ( isPullRequestConfigured ) { | ||
| returnVal = { pullRequest: config.pullRequest }; |
There was a problem hiding this comment.
why not return directly?
src/helpers/file.helpers.ts
Outdated
| } | ||
|
|
||
| export function isPullRequestOrBranchConfigured(config: Config) { | ||
| let returnVal = {}; |
src/helpers/file.helpers.ts
Outdated
| returnVal = { pullRequest: config.pullRequest }; | ||
| } | ||
| } | ||
| return ( |
| returnVal = { branch: config.branch }; | ||
| } | ||
|
|
||
| // Give pullRequest highest priority |
There was a problem hiding this comment.
if you want to give highest priority you can move before the if ( isBranchConfigured ) { and return directly
There was a problem hiding this comment.
Hi @pedroresende ,
I refactored changes. Thanks for the tip.
lukzeg
force-pushed
the
feature/branch_and_pullRequest_analysis
branch
from
2e5eace to
6ac46c8
Compare
src/helpers/file.helpers.ts
Outdated
| if ( isPullRequestConfigured ) { | ||
| return { pullRequest: config.pullRequest }; | ||
| } | ||
| else { |
There was a problem hiding this comment.
nbd: you don't need the else
lukzeg
force-pushed
the
feature/branch_and_pullRequest_analysis
branch
from
6ac46c8 to
396ae96
Compare
|
@lukzeg It's really a good enhancement to have. Here are my thoughts on the approach. I personally feel that this should be easier for the user, i.e by a dropdown or something instead of having to update the config file every time. The config file should be considered as a one-time setup process. What do y'all think? |
lukzeg
force-pushed
the
feature/branch_and_pullRequest_analysis
branch
from
396ae96 to
ba74af1
Compare
|
Hi @adisreyaj , @pedroresende , The idea to create settings part sound really great. I think it will be great to create some milestone with such of feature. So anyone who is interested might start to work on it. I would like to avoid of making such of change in current PRst, just to simplify the commits and parts which are delivered. I know that might be a strange, but small parts of code are much more easier to be Code Reviewed. What do you think about creating some milestones, with ideas of future improvements required/ request by you and other users? |
lukzeg
force-pushed
the
feature/branch_and_pullRequest_analysis
branch
3 times, most recently
from
a143e92 to
286709c
Compare
Update README.md with information about optional arguments: - branch - pullRequest
Add errorMsg when pullRequest or branch is not longer available at SonarQube server
lukzeg
force-pushed
the
feature/branch_and_pullRequest_analysis
branch
from
286709c to
d56e487
Compare
Hi @adisreyaj , @pedroresende
I would like to propose small extension to your plugin. I would like to extend it via adding possibility to collect metrics from pullRequest or branch analysis.
For instance if the end user would like to receive analysis from pullRequest, he might define new attribute pullRequest:
{ "project": "adisreyaj_compito", "sonarURL": "https://sonarcloud.io", "pullRequest": 212, "auth": { "username": "", "password": "" } }or branch:
{ "project": "adisreyaj_compito", "sonarURL": "https://sonarcloud.io", "branch": "bugfix/FixMe", "auth": { "username": "", "password": "" } }In case if both of attrs are added, the highest priority will have pullRequest, as mostly engineers will be interested to see potential issues which they deliver to master branch.
Please let me know if this feature looks okay for you.
Kind Regards,
Lucas