singa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Moaz Reyad <m...@apache.org>
Subject Re: [DISCUSS] Improve code quality
Date Wed, 04 Mar 2020 10:13:04 GMT
Dear all,

Here are two updates about lgtm:

1. Just few days after our discussion, GitHub announced that they acquired Semmle (the company
that made lgtm). So lgtm is now part of GitHub. (See https://github.blog/2019-09-18-github-welcomes-semmle/)

2. Apache INFRA now has lgtm installed in the Apache GitHub organization account. We can enable
the lgtm pull request integration on SINGA repo if everyone agrees. 

Thanks
Moaz

On 2019/09/16 04:20:43, Moaz Reyad <moaz@apache.org> wrote: 
> Thank you all for participating in the discussion. It is nice that everyone
> agrees that code quality is important.
> 
> Here are some comments:
> 
> 1. It seems that everyone agrees on cpplint and pylint, but no one
> discussed lgtm. Do you agree on using also the lgtm tool which is proposed
> in [1] ? or do you think it is not needed and the cpplint and pylint is
> enough? Note that there are lgtm badges that can be added to SINGA readme
> on Github and acts as a certificate of the code quality that is visible to
> everyone. While cpplint and pylint are not as visible as lgtm. Also lgtm
> seems to report other problems that cpplint and pylint do not recognize.
> Please share your ideas about the lgtm tool and SINGA-484 [1] issue.
> 
> 2. We need to separate the discussion of the problem of future code quality
> and the problem of current code quality. Enforcing code quality in future
> is great, but in my opinion the team must take some actions also for the
> current code. There are hundreds of issues reported by the three tools as
> mentioned in my previous email. Not all of them are important of course,
> but some percentage of them need to be fixed.
> 
> 3. For future code quality, it is a good idea to use continuous integration
> to run static code checking (cpplint, pylint and also RAT). I can open a
> ticket in jira for this and anyone is welcome to implement it (after
> finishing the more urgent issues with Travis CI). But cpplint and pylint
> test will show too many errors already on the current code in the master
> branch. This is why I think we need to solve the problem of current code
> quality before enforcing future code quality.
> 
> 4. The official site of SINGA is [2]. When the user navigates to "How to
> Contribute Code" link, he will go to [3]. The link that was mention in [4]
> is not accessible from [2]. The English site was moved to the root as the
> default language [5], so no need to use "/en" folder in the path. If you
> use the latest site build script [6], it will not create "/en" folder. This
> means the link [4] was not generated by the latest site build script [6].
> 
> 5. The code style instructions in [4] can be useful only for code style
> issues, but all other code problems (such as those hundreds of issues
> reported by the three tools) are not only about code style. Check the LGTM
> alerts for SINGA [7] for example, they are not code style or readability
> issues. (e.g unused variables, unreachable code, ..)
> 
> best regards,
> Moaz
> 
> [1] https://issues.apache.org/jira/projects/SINGA/issues/SINGA-484
> [2] http://singa.apache.org
> [3] http://singa.apache.org/develop/contribute-code.html
> [4] http://singa.apache.org/en/develop/contribute-code.html
> [5] https://issues.apache.org/jira/projects/SINGA/issues/SINGA-436
> [6] https://github.com/apache/incubator-singa/blob/master/doc/build.sh
> [7] https://lgtm.com/projects/g/apache/incubator-singa/alerts/?mode=list
> 
> On Sun, Sep 15, 2019 at 5:02 PM Yeung Sai Ho <dcsysh@nus.edu.sg> wrote:
> 
> > P.S. Therefore, tools like cpplint and pylint are useful and can be
> > integrated into CI process that can check code automatically.
> >
> > 從我的 iPhone 傳送
> >
> > Yeung Sai Ho <dcsysh@nus.edu.sg<mailto:dcsysh@nus.edu.sg>> 於 2019年9月15日
> > 下午10:18 寫道:
> >
> >
> > Dear Prof. Moaz Reyad,
> >
> >
> >
> > Nice to discuss with you, I am currently with NUS as a research fellow,
> > and working with my peer co-workers (e.g. shicong/dcslin, joddly, shicheng,
> > wanqi, pinpom, etc.) on SINGA leaded by our boss. I am Chris Yeung and this
> > is my first time to join the discussion here.
> >
> >
> >
> > I agree with you. As a researcher and developer, I totally feel the same
> > since code quality is very important. The code analysis tool could save our
> > time and improve coding quality.
> >
> >
> >
> > In this email, please let me share my own personal view, concerning some
> > recent daily practice in our local team which is currently using to enhance
> > code quality:
> >
> >
> >
> > We are currently using the tools like cpplint and pylint to check and
> > reformat the code so that we make sure our code is in compliance with the
> > google style.
> >
> >
> >
> > Concerning our SINGA website, the new developers are suggested to read the
> > following SINGA doc web page before they join developing the SINGA, which
> > gives a complete and useful guide for their development:
> >
> > https://singa.apache.org/en/develop/contribute-code.html
> >
> >
> >
> > The above website recommends a very simple approach to enforce the Google
> > coding styles: We can use the VS Code editor (
> > https://code.visualstudio.com) and set the linting and formating tools.
> > First, we need to install the C/C++ extension and python extension. Then,
> > we can edit the seetings.json as:
> >
> > "editor.formatOnSave": true,
> >
> > "python.formatting.provider": "yapf",
> >
> > "python.formatting.yapfArgs": [
> >
> >     "--style",
> >
> >     "{based_on_style: google}"
> >
> > ],
> >
> > "python.linting.enabled": true,
> >
> > "python.linting.lintOnSave": true,
> >
> > "C_Cpp.clang_format_style": "Google"
> >
> >
> >
> > A reference settings.json file can be found here:
> >
> > https://gist.github.com/nudles/3d23cfb6ffb30ca7636c45fe60278c55
> >
> >
> >
> > The developers are suggested to fix the format errors before submitting
> > the PRs (pull requests).
> >
> >
> >
> > In addition to the code quality improving tool, we also have the tools for
> > adding documentations. The procedures to contribute for the documentation
> > is in our SINGA doc web page:
> >
> > https://singa.apache.org/en/develop/contribute-docs.html
> >
> >
> >
> > In our recent development, we always perform peer discussion in the local
> > team, formally meeting with our boss almost everyday, and seek advise from
> > our boss whenever any problems arise. We learn from each other and improve
> > ourselves gradually.
> >
> >
> >
> > The above is just to share my own personal view, concerning our recent
> > experience that also concerns code quality. Thank you very much for
> > listening!
> >
> >
> >
> > Best Regards,
> >
> > Research Fellow
> >
> > Chris YEUNG Sai Ho (
> > https://scholar.google.com.sg/citations?user=9XAJsd0AAAAJ&hl=en and
> > https://github.com/chrishkchris)
> >
> >
> >
> > Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for
> > Windows 10
> >
> >
> >
> > From: Wang Wei<mailto:wangwei@comp.nus.edu.sg>
> > Sent: Sunday, 15 September 2019 2:38 PM
> > To: dev@singa.incubator.apache.org<mailto:dev@singa.incubator.apache.org>
> > Cc: dev@singa.apache.org<mailto:dev@singa.apache.org>
> > Subject: Re: [DISCUSS] Improve code quality
> >
> >
> >
> > Hi Moaz,
> >
> > I agree with you.
> > We did take some efforts to improve the code quality.
> > [1] introduces some tools for enforcing the coding style.
> > [2] introduces some tools for adding documentations.
> >
> > The current issue is that our contributors may have applied different
> > coding styles and tools using their different editors.
> > I suggest to do some tests during the CI process, e.g., running the cpplint
> > and pylint.
> > If all tests pass, then we merge the PR.
> >
> > Thanks!
> >
> > Best,
> > Wei
> >
> > [1]. http://singa.apache.org/en/develop/contribute-code.html
> > [2]. http://singa.apache.org/en/develop/contribute-docs.html
> >
> > On Sun, Sep 15, 2019 at 2:36 AM Moaz Reyad <moaz@apache.org<mailto:
> > moaz@apache.org>> wrote:
> >
> > > Dear team,
> > >
> > > Since SINGA is going to graduate soon from the incubator, I propose to
> > use
> > > some tools to ensure high code quality. These tools check for known
> > > problems in the code and provide a detailed report for fixing them. May
> > be
> > > some code came from scientific experimental projects. We need to improve
> > > this code according to industry standards, so it can be used with more
> > real
> > > life projects.
> > >
> > > 1. I propose to add the code quality tools (cpplint[1], pylint[2] and
> > > lgtm[3]) to SINGA contribution guideline[4], so that each developer is
> > > encouraged to install and run code quality checks in his local repo and
> > fix
> > > any problems before creating a pull request.
> > >
> > > 1.A CPP Lint: running cpplint in the src directory shows 822 errors,
> > while
> > > running in the include directory shows 708 errors. The guidelines [4] has
> > > an outdated information that instructs developers to use an old
> > > non-existing file tool/cpplint.py.
> > >
> > > 1.B Python Lint: running pylint in python/singa shows 5.00/10 rating,
> > while
> > > running in python/rafiki shows 0.00/10 rating.
> > >
> > > 1.C LGTM :There is a Jira ticket for adding LGTM badges to the README[5],
> > > so the quality of the code becomes more clear to everyone. LGTM pull
> > > request automation can't be enabled in Apache repo due to infra
> > > restrictions[6], but it works on personal forks of the project. Currently
> > > LGTM rates both C++ and Python code in SINGA as grade D.
> > >
> > > 2. I propose also to give the code quality higher priority in the next
> > > release since it is probably going to be the first release after
> > > graduation. The team is invited to fix as much as possible from the
> > current
> > > code issues and to use tools that check their new code before pushing it
> > to
> > > SINGA. Let's try to make the lgtm grade and lint rating as high as
> > > possible.
> > >
> > > Improving code quality is required to attract new users and developers.
> > > Users will trust more the project with better code and developers will be
> > > happy to contribute to it. It will also make the code review process
> > easier
> > > and more productive instead of wasting time in finding and fixing known
> > > code problems.
> > >
> > > New developers (or old developers who did not contribute for a while and
> > > would like to warm up) can start working on fixing lgtm and lint issues,
> > > since they are usually easy and there is a clear explanation of the
> > problem
> > > and how to solve it.
> > >
> > > What do you think?
> > >
> > > p.s. This discussion is the first topic in a series of proposals to
> > improve
> > > SINGA as it will be an Apache top level project soon. The next proposal
> > > will discuss improving the build and test pipeline in a separate thread
> > to
> > > avoid discussing too many things in one thread.
> > >
> > > best regards,
> > > Moaz
> > >
> > > [1] https://pypi.org/project/cpplint/
> > > [2] https://www.pylint.org/
> > > [3] https://lgtm.com/
> > > [4] http://singa.apache.org/develop/contribute-code.html
> > > [5] https://issues.apache.org/jira/projects/SINGA/issues/SINGA-484
> > > [6] https://issues.apache.org/jira/browse/INFRA-17954
> > >
> >
> > ________________________________
> >
> > Important: This email is confidential and may be privileged. If you are
> > not the intended recipient, please delete it and notify us immediately; you
> > should not copy or use it for any purpose, nor disclose its contents to any
> > other person. Thank you.
> >
> 

Mime
View raw message