Closed bradcray closed 2 years ago
i am ok to outlaw TABs, I use 4-space indents which is what 95% of the arkouda code uses. Bill seems to use 2-space indents sometimes and I re-indent things as I edit them. I also like UNIX style EOLs.
Yeah, let's outlaw TABs and enforce UNIX EOLs. Like Brad, I prefer 2-space indents, but when I am editing Mike's code I sometimes try to respect his 4-space indents, even though it hurts.
On Tue, Oct 29, 2019 at 10:15 PM Michael Merrill notifications@github.com wrote:
i am ok to outlaw TABs, I use 4-space indents which is what 95% of the arkouda code uses. Bill seems to use 2-space indents sometimes and I re-indent things as I edit them. I also like UNIX style EOLs.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mhmerrill/arkouda/issues/168?email_source=notifications&email_token=AENPBSPEERZKHBKMNEIU7MDQRDU33A5CNFSM4JGR6RU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECSV7OY#issuecomment-547708859, or unsubscribe https://github.com/notifications/unsubscribe-auth/AENPBSMG7VKD2THVC3HB73DQRDU33ANCNFSM4JGR6RUQ .
I think we have some scripts in Chapel to enforce the "no TABs" rule. I can refresh my memory how that's done to see whether it could be re-used for Arkouda if that would be of interest. In the meantime, I'll stop worrying about preserving them when I come across them (emacs users can avoid having them be inserted by dropping:
(setq-default indent-tabs-mode nil)
into their .emacs
file.
Ooo, thanks for the incantation.
On Wed, Oct 30, 2019, 8:03 PM Brad Chamberlain notifications@github.com wrote:
I think we have some scripts in Chapel to enforce the "no TABs" rule. I can refresh my memory how that's done to see whether it could be re-used for Arkouda if that would be of interest. In the meantime, I'll stop worrying about preserving them when I come across them (emacs users can avoid having them be inserted by dropping:
(setq-default indent-tabs-mode nil)
into their .emacs file.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mhmerrill/arkouda/issues/168?email_source=notifications&email_token=AENPBSLUBZCRCJDGJDYP523QRIODLA5CNFSM4JGR6RU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECWEUJY#issuecomment-548162087, or unsubscribe https://github.com/notifications/unsubscribe-auth/AENPBSLMYO7VHHOS5E6QWQLQRIODLANCNFSM4JGR6RUQ .
Refreshing my memory about what we do to keep ourselves honest in our code base, I don't think we have anything so readily adoptable that you couldn't just as easily build it yourself. In our repo, we've got a script, lookForTabs
that essentially git grep
s sources of interest for tabs. There are a number of ways such a script could be run:
As part of the make
process (upside: trivial to implement; downside: adds overhead for each build, but maybe a negligible amount?)
As a cron job: We used to do this when we ran testing as a cron job (upside: still pretty easy; downside: relies on cron
which seems increasingly old school and hard to maintain; requires a machine resource you can run cron on; will only catch problems after merging)
As a smoke test that will trigger each time a merge is made to your master repository: This is what we do today using Jenkins (BuiltBot could be an alternative). (upside: this is considered standard practice in many circles; downside: Jenkins is pretty heavyweight to get started with; and you won't catch issues until after merges (though hopefully this is a rare occurrence)).
As a Travis job: This is what I think I would do if I ran arkouda: Travis is a utility that can run simple sanity checks on a PR to save time before merging and I think is fairly lightweight to get started with. It runs on community resources, so only lightweight jobs should be put there, but checking for tabs fits that bill.
I also would like to propose that we don't use single statement Chapel forms like then
and do
. This has tripped me up several times creating several hard-to-find subtle bugs. Also the emacs chapel-mode is quirky with indenting then
sometimes as well as other things.
In my opinion it is just as easy to use if expr {stmt;}
as if expr then stmt;
After reading the Python PEP-8 coding standards, I think we should stick/adopt as close to these as possible even for the Chapel code. PEP-8 is here: https://www.python.org/dev/peps/pep-0008/
https://github.com/mhmerrill/arkouda/pull/240 adds checks for tabs in Chapel files to the CI testing.
We can almost certainly add a CI job to run flake8, pylint, or whatever your favorite linter/style-checker is for the python code.
@pierce314159 and I were discussing this issue and believe that we can close this, but open a new issue to update CONTRIBUTING.md
in Arkouda to detail the coding standards that should be utilized in python and arkouda. Recommend Clossing this issue and working updates under #1301.
i concur with closing this.
As I edit arkouda code, something I'm tripping over regularly relates to differences in whitespace:
Most editors can be configured to support variations on the above, and I think it'd be helpful to developers to establish a consistent approach (and maybe have tools to guard against introducing violations of easy cases like bullets 1 and 3). Specifically, I've found myself wrestling to get TABs and linefeeds back as similar as in the original source as possible to minimize the diff that my PR is introducing, which seems like a waste of time.
My preferences:
(of these, the second is the one I feel least strongly about)