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

Improve our CodeFactor mark #639

Closed
mossroy opened this issue Jul 11, 2020 · 18 comments · Fixed by #663
Closed

Improve our CodeFactor mark #639

mossroy opened this issue Jul 11, 2020 · 18 comments · Fixed by #663

Comments

@mossroy
Copy link
Contributor

mossroy commented Jul 11, 2020

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.

@mossroy mossroy added this to the v2.9 milestone Jul 11, 2020
@Jaifroid
Copy link
Member

I agree. Codefactor even offers to make PRs for simple fixes. Might be worth experimenting with that, if the proposed fixes are sensible.

@Jaifroid Jaifroid modified the milestones: v3.0, v3.1 Oct 3, 2020
@ram690514
Copy link
Contributor

@mossroy i would like to work on this issue

@ram690514
Copy link
Contributor

@kelson42 i would like to work on this issue

@kelson42
Copy link
Collaborator

@ram690514 You are welcome.

@mossroy
Copy link
Contributor Author

mossroy commented Oct 20, 2020

@ram690514 : I'd prefer that you finalize work on #660 PR first, before starting work on another issue.

ram690514 added a commit to ram690514/kiwix-js that referenced this issue Oct 20, 2020
@ram690514
Copy link
Contributor

@mossroy :I am working on it . I will fix it surely as fast as possible.

@ram690514
Copy link
Contributor

@kelson42 : I have pushed a pull request . please check it

@ram690514
Copy link
Contributor

@mossroy : I have pushed a pull request. can you please check it

@Jaifroid
Copy link
Member

@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!

@ram690514
Copy link
Contributor

@Jaifroid I will surely do it

@ram690514
Copy link
Contributor

I have created a pull request regarding this issue..please check @kelson42

@ram690514
Copy link
Contributor

@Jaifroid : I have pushed a pull request.please check it.

@Jaifroid
Copy link
Member

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.

@ram690514
Copy link
Contributor

ram690514 commented Oct 22, 2020 via email

@ram690514
Copy link
Contributor

@Jaifroid : I have updated the pull request ,please check it

@ram690514
Copy link
Contributor

@kelson42 : I have updated the pull request,please check it

@kelson42 kelson42 linked a pull request Oct 22, 2020 that will close this issue
@ram690514
Copy link
Contributor

I had restored the element message channel and updated the pull request

@Jaifroid
Copy link
Member

Jaifroid commented Nov 7, 2020

Well, I was wrong. Merging this went from a score of C- to a score of B !

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

Successfully merging a pull request may close this issue.

4 participants