ShivamPokhriyal
commented
3 years ago @barbosa Can you take a look at this?
Closed ShivamPokhriyal closed 3 years ago
ShivamPokhriyal
commented
3 years ago @barbosa Can you take a look at this?
barbosa
commented
3 years ago Hey @ShivamPokhriyal - thank you very much for working on this. Would you want to include a few unit tests guaranteeing the logic is correct too? I can guide you through if you need any help.
ShivamPokhriyal
commented
3 years ago Would you want to include a few unit tests guaranteeing the logic is correct too? I can guide you through if you need any help.
I had a quick look into the test setup and it seems like this is being used as the git diff. So, I'm thinking of adding a xml containing lint issues for the files in that diff and then write tests using that xml. Is this the correct way?
barbosa
commented
3 years ago I wonder if this shouldn't be covering that case. May we need to double-check what that test is actually doing.
ShivamPokhriyal
commented
3 years ago I wonder if this shouldn't be covering that case. May we need to double-check what that test is actually doing.
Looked into this test case and now I'm wondering about the actual meaning of filtering parameter.
If filtering simply means that it'll list all the lint issues in the modified files(even the issues at lines which aren't changed but are present in the same file), then that test case makes sense, and we'll probably need to create another parameter which will warn issues only for modified lines(and not modified files).
Otherwise, the test case isn't enough, it tests that when filtering is used, lint issues of only modified files are listed, but doesn't check whether the issues are of modified lines or unmodified lines of the modified file.
So, should we modify the test or create another parameter?
ShivamPokhriyal
commented
3 years ago @barbosa bumping again
barbosa
commented
3 years ago @ShivamPokhriyal I'd say it would be better to have another param as the original filtering one states it is for modified files.
Checking "lines" only would have to have higher priority than "files" though.
ShivamPokhriyal
commented
3 years ago Makes sense! Any ideas on the name of the new parameter?
barbosa
commented
3 years ago filtering_lines?
On Mon, Jun 7, 2021 at 11:02 AM Shivam Pokhriyal @.***> wrote:
Makes sense! Any ideas on the name of the new parameter?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/loadsmart/danger-android_lint/pull/30#issuecomment-856012846, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZNSDV5UROZSANCH6JTR3TRTNRXANCNFSM45KONVOQ .
ShivamPokhriyal
commented
3 years ago @barbosa Another round of review please. I'm not sure how to run this plugin locally to test the changes, can you help me with that?
ShivamPokhriyal
commented
3 years ago I'm not sure how to run this plugin locally to test the changes, can you help me with that?
Got it, I just have to specify local path in gemfile.
barbosa
commented
3 years ago @ShivamPokhriyal did it work as expected when running with the changes locally? Lmk and I can merge this and submit a new version of the plugin.
ShivamPokhriyal
commented
3 years ago Ran it locally on this PR https://github.com/dimagi/commcare-android/pull/2501 and it worked fine. @barbosa
➜ commcare-android git:(dummy-lint) ✗ bundle exec danger pr https://github.com/dimagi/commcare-android/pull/2501
Running your Dangerfile against this PR - https://github.com/dimagi/commcare-android/pull/2501
Turning on --verbose
Info:
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Danger v8.2.3 |
| DSL Attributes |
+------------------+------------------------------------------------------------------------------------------------------------------------------------------------+
| violation_report | |
| status_report | |
| --- | --- |
| scm_provider | github |
| tags | #<Enumerator:0x00007f940b970328> |
| commits | b67bf14163879f9fb09208858b78318e65c45843 |
| added_files | #<Danger::FileList:0x00007f940a9a3710> |
| deleted_files | #<Danger::FileList:0x00007f940a9a30d0> |
| modified_files | app/src/org/commcare/activities/AppManagerActivity.java |
| | app/src/org/commcare/activities/CommCareWiFiDirectActivity.java |
| renamed_files | [] |
| lines_of_code | 32 |
| deletions | 26 |
| insertions | 6 |
| diff | diff --git a/app/src/org/commcare/activities/AppManagerActivity.java b/app/src/org/commcare/activities/AppManagerActivity.java |
| | index 8be13f101..6299c3350 100644 |
| | --- a/app/src/org/commcare/activities/AppManagerActivity.java |
| | +++ b/app/src/org/commcare/activities/AppManagerActivity.java |
| | @@ -3,6 +3,7 @@ package org.commcare.activities; |
| | import android.content.DialogInterface; |
| | import android.content.Intent; |
| | import android.os.Bundle; |
| | +import android.util.Log; |
| | import android.view.Menu; |
| | import android.view.MenuItem; |
| | import android.view.View; |
| | @@ -47,6 +48,7 @@ public class AppManagerActivity extends CommCareActivity implements OnItemClickL |
| | @Override |
| | protected void onCreate(Bundle savedInstanceState) { |
| | super.onCreate(savedInstanceState); |
| | + Log.d("Shivamm", "Should give lint issues"); |
| | setContentView(R.layout.app_manager); |
| | ((ListView)this.findViewById(R.id.apps_list_view)).setOnItemClickListener(this); |
| | FirebaseAnalyticsUtil.reportAppManagerAction(AnalyticsParamValue.OPEN_APP_MANAGER); |
| | @@ -90,10 +92,7 @@ public class AppManagerActivity extends CommCareActivity implements OnItemClickL |
| | } |
| | |
| | protected void showAboutCommCareDialog() { |
| | - CommCareAlertDialog dialog = DialogCreationHelpers.buildAboutCommCareDialog(this, false); |
| | - dialog.makeCancelable(); |
| | - dialog.setOnDismissListener(dialog1 -> handleDeveloperModeClicks()); |
| | - showAlertDialog(dialog); |
| | + Log.d("Shivamm", "Another lint issue"); |
| | } |
| | |
| | private void handleDeveloperModeClicks() { |
| | diff --git a/app/src/org/commcare/activities/CommCareWiFiDirectActivity.java b/app/src/org/commcare/activities/CommCareWiFiDirectActivity.java |
| | index f3f02913d..464f027e3 100644 |
| | --- a/app/src/org/commcare/activities/CommCareWiFiDirectActivity.java |
| | +++ b/app/src/org/commcare/activities/CommCareWiFiDirectActivity.java |
| | @@ -110,6 +110,7 @@ public class CommCareWiFiDirectActivity |
|
|
|
|"Adding lint issues in class which already have a lot of lint issues");
|
|t);
|_text);
| | @@ -808,24 +809,7 @@ public class CommCareWiFiDirectActivity |
|
|
|yFile(InputStream inputStream, OutputStream out) throws IOException {
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|ile Server failed with exception " + e, e);
|
|
|
|
|
|
|
| | @@ -836,10 +820,7 @@ public class CommCareWiFiDirectActivity |
|
|
|
|
|Channel, (PeerListListener)this.getSupportFragmentManager()
|
|
|
|
|
| | @Override |
| mr_body | ## Summary |
| | |
| | |
| | <!-- |
| | |
| | |
| | Provide a link to the ticket or document which prompted this change, |
| | |
| | |
| | Describe the rationale and design decisions. |
| | |
| | |
| | --> |
| | |
| | |
| | |
| | |
| | |
| | ## Feature Flag |
| | |
| | |
| | <!-- If this is specific to a feature flag, which one? --> |
| | |
| | |
| | |
| | |
| | |
| | ## Product Description |
| | |
| | |
| | <!-- For non-invisible changes, describe user-facing effects. Would be good to a |
| | dd screenshots/videos for any major user facing changes --> |
| | |
| | |
| | |
| | |
| | |
| | ## Safety Assurance |
| | |
| | |
| | |
| | |
| | |
| | - [ ] If the PR is high risk, "High Risk" label is set |
| | |
| | |
| | - [ ] I have confidence that this PR will not introduce a regression for the rea |
| | sons below |
| | |
| | |
| | - [ ] Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is |
| | set correctly |
| | |
| | |
| | |
| | |
| | |
| | ### Automated test coverage |
| | |
| | |
| | |
| | |
| | |
| | <!-- Identify the related test coverage and the tests it would catch --> |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | ### Safety story |
| | |
| | |
| | <!-- |
| | |
| | |
| | Describe any other pieces to the safety story including |
| | |
| | |
| | local testing, why the change is inherently safe, and/or plans to limit the blas |
| | t radius of a defect. |
| | |
| | |
| | --> |
| mr_author | ShivamPokhriyal |
| mr_labels | don't pull |
| | skip-integration-tests |
| pr_json | [Skipped JSON] |
| mr_json | [Skipped JSON] |
| review | #<Danger::RequestSources::GitHubSource::Review:0x00007f940a10d9e8> |
| pr_title | Dummy lint issue |
| pr_body | ## Summary |
| | |
| | |
| | <!-- |
| | |
| | |
| | Provide a link to the ticket or document which prompted this change, |
| | |
| | |
| | Describe the rationale and design decisions. |
| | |
| | |
| | --> |
| | |
| | |
| | |
| | |
| | |
| | ## Feature Flag |
| | |
| | |
| | <!-- If this is specific to a feature flag, which one? --> |
| | |
| | |
| | |
| | |
| | |
| | ## Product Description |
| | |
| | |
| | <!-- For non-invisible changes, describe user-facing effects. Would be good to a |
| | dd screenshots/videos for any major user facing changes --> |
| | |
| | |
| | |
| | |
| | |
| | ## Safety Assurance |
| | |
| | |
| | |
| | |
| | |
| | - [ ] If the PR is high risk, "High Risk" label is set |
| | |
| | |
| | - [ ] I have confidence that this PR will not introduce a regression for the rea |
| | sons below |
| | |
| | |
| | - [ ] Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is |
| | set correctly |
| | |
| | |
| | |
| | |
| | |
| | ### Automated test coverage |
| | |
| | |
| | |
| | |
| | |
| | <!-- Identify the related test coverage and the tests it would catch --> |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | ### Safety story |
| | |
| | |
| | <!-- |
| | |
| | |
| | Describe any other pieces to the safety story including |
| | |
| | |
| | local testing, why the change is inherently safe, and/or plans to limit the blas |
| | t radius of a defect. |
| | |
| | |
| | --> |
| pr_author | ShivamPokhriyal |
| pr_labels | don't pull |
| | skip-integration-tests |
| branch_for_base | master |
| branch_for_head | dummy-lint |
| base_commit | 8dcbfeb44e774143428de579962def5038d77e8e |
| head_commit | b67bf14163879f9fb09208858b78318e65c45843 |
| api | Octokit::Client |
| pr_diff | [Skipped Diff] |
| mr_title | Dummy lint issue |
| filtering | |
| filtering_lines | |
| severity | Warning |
| report_file | app/build/reports/lint/lint-result.xml |
| gradle_task | lint |
| skip_gradle_task | false |
| --- | --- |
| SCM | Danger::GitRepo |
| Source | Danger::LocalGitRepo |
| Requests | Danger::RequestSources::GitHub |
| Base Commit | commit 8dcbfeb44e774143428de579962def5038d77e8e |
| | Merge: 2410ea7ab fd97cc14c |
| | Author: Shivam Pokhriyal <shivampokhriyal1997@gmail.com> |
| | Date: Mon Jun 14 12:55:45 2021 +0530 |
| | |
| | Merge pull request #2499 from dimagi/refractor-draw-view |
| | |
| | Move DrawView to separate file |
| Head Commit | commit b67bf14163879f9fb09208858b78318e65c45843 |
| | Author: ShivamPokhriyal <shivampokhriyal1997@gmail.com> |
| | Date: Wed Jun 16 13:38:19 2021 +0530 |
| | |
| | Dummy lint issue |
+------------------+------------------------------------------------------------------------------------------------------------------------------------------------+
Results:
Warnings:
- [ ] This PR does not contain any JIRA issue keys in the PR title or commit messages (e.g. KEY-123)
- [ ] app/src/org/commcare/activities/AppManagerActivity.java#L51: Using 'Log' instead of 'Timber'
- [ ] app/src/org/commcare/activities/AppManagerActivity.java#L95: Using 'Log' instead of 'Timber'
- [ ] app/src/org/commcare/activities/CommCareWiFiDirectActivity.java#L113: Using 'Log' instead of 'Timber'
- [ ] app/src/org/commcare/activities/CommCareWiFiDirectActivity.java#L812: Using 'Log' instead of 'Timber'
- [ ] app/src/org/commcare/activities/CommCareWiFiDirectActivity.java#L823: Using 'Log' instead of 'Timber'Just noticed that earlier I was using filtering_lines along with filtering. I updated my dangerfile to only have filtering_lines param and it gives some error. Here's my Dangerfile
android_lint.report_file = "app/build/reports/lint-results-commcareRelease.xml"
android_lint.skip_gradle_task = true
android_lint.filtering_lines = true
android_lint.lint(inline_mode: true)The error I get is
bundler: failed to load command: danger (/Users/shivampokhriyal/.rbenv/versions/3.0.0/bin/danger)
/Users/shivampokhriyal/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/git-1.8.1/lib/git/diff.rb:64:in `[]': \e[31m (Danger::DSLError)
[!] Invalid `Dangerfile` file: undefined method `[]' for nil:NilClass\e[0m
# from Dangerfile:28
# -------------------------------------------
# android_lint.filtering = false
> android_lint.lint(inline_mode: true)
# -------------------------------------------
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:204:in `block (2 levels) in send_inline_comment'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:198:in `each'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:198:in `block in send_inline_comment'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:195:in `each'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:195:in `send_inline_comment'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:122:in `lint'
from Dangerfile:28:in `eval_file'Will look more into it tomorrow. But just wanted to flag it here to not merge the PR yet.
ShivamPokhriyal
commented
3 years ago Just noticed that earlier I was using filtering_lines along with filtering. I updated my dangerfile to only have filtering_lines param and it gives some error. Here's my Dangerfile
android_lint.report_file = "app/build/reports/lint-results-commcareRelease.xml"
android_lint.skip_gradle_task = true
android_lint.filtering_lines = true
android_lint.lint(inline_mode: true)The error I get is
bundler: failed to load command: danger (/Users/shivampokhriyal/.rbenv/versions/3.0.0/bin/danger)
/Users/shivampokhriyal/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/git-1.8.1/lib/git/diff.rb:64:in `[]': \e[31m (Danger::DSLError)
[!] Invalid `Dangerfile` file: undefined method `[]' for nil:NilClass\e[0m
# from Dangerfile:28
# -------------------------------------------
# android_lint.filtering = false
> android_lint.lint(inline_mode: true)
# -------------------------------------------
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:204:in `block (2 levels) in send_inline_comment'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:198:in `each'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:198:in `block in send_inline_comment'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:195:in `each'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:195:in `send_inline_comment'
from /Users/shivampokhriyal/Documents/projects/danger-android_lint/lib/android_lint/plugin.rb:122:in `lint'
from Dangerfile:28:in `eval_file'Will look more into it tomorrow. But just wanted to flag it here to not merge the PR yet.
ShivamPokhriyal
commented
3 years ago The error I get is
Ohh nevermind, I think it was happening because git diff was using my local diff and not for PR. But now, with filtering_lines set to true and filtering set to false, it simply prints all the issues in the repo because of this check https://github.com/loadsmart/danger-android_lint/pull/30/files#diff-0d7e373d04e96e69ed2571afc6348abb2dfc5ff390982cc0bcc6e0f167cac92bR201 which always evaluates to true since filtering is false.
next unless !filtering || (target_files.include? filename)probably need to change that to something like next unless (!filtering && !filtering_lines) || (target_files.include? filename)
Will need to improve the test case as well.
ShivamPokhriyal
commented
3 years ago @barbosa added the changes. Another round of review please.
ShivamPokhriyal
commented
3 years ago @barbosa bumping you again.
barbosa
commented
3 years ago This is now available on v0.0.9 Thank you for your contribution, @ShivamPokhriyal 👍
Addresses https://github.com/loadsmart/danger-android_lint/issues/18