-
-
Notifications
You must be signed in to change notification settings - Fork 149
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
Improve our CodeFactor mark #639
Comments
I agree. Codefactor even offers to make PRs for simple fixes. Might be worth experimenting with that, if the proposed fixes are sensible. |
@mossroy i would like to work on this issue |
@kelson42 i would like to work on this issue |
@ram690514 You are welcome. |
@ram690514 : I'd prefer that you finalize work on #660 PR first, before starting work on another issue. |
@mossroy :I am working on it . I will fix it surely as fast as possible. |
@kelson42 : I have pushed a pull request . please check it |
@mossroy : I have pushed a pull request. can you please check it |
@ram690514 Thank you, but it's impossible to see what you have actually fixed from that PR. Could you please close it and re-do it using with only the actual code changes, or perhaps with one commit per change, Please do not change formatting and indenting, only focus on the actual code changes. If the code change isn't clearly shown in the diff, we cannot see what you have done. and so cannot check your work! |
@Jaifroid I will surely do it |
I have created a pull request regarding this issue..please check @kelson42 |
@Jaifroid : I have pushed a pull request.please check it. |
I've reviewed it and suggested changes. As a matter of etiquette, when you create a PR, please give it an informative title ("Changes" means nothing and could apply to any PR), and also provide an overview description of what you have done. You don't need to provide a description now, as we know what you have done, but it is good practice for a PR. You should change the PR title, however. |
Thanks for your your suggestions, I will surely do it
…On Thu, Oct 22, 2020 at 12:13 PM Jaifroid ***@***.***> wrote:
I've reviewed it and suggested changes. As a matter of etiquette, when you
create a PR, please give it an informative title ("Changes" means nothing
and could apply to any PR), and also provide an overview description of
what you have done. You don't need to provide a description now, as we know
what you have done, but it is good practice for a PR. You should change the
PR title, however.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#639 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ARAWVBRZ4UIF23XGTTIYVQTSL7H7RANCNFSM4OXJYSXA>
.
|
@Jaifroid : I have updated the pull request ,please check it |
@kelson42 : I have updated the pull request,please check it |
I had restored the element message channel and updated the pull request |
Well, I was wrong. Merging this went from a score of C- to a score of B ! |
CodeFactor does not give us a very good mark for now : C-
especially for app.js (F) : https://www.codefactor.io/repository/github/kiwix/kiwix-js/file/master/www/js/app.js
There are many minor issues that could easily be fixed.
And we also might configure it to ignore the external libraries.
The text was updated successfully, but these errors were encountered: