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

Set up auto-formatting of Python #1

Open
FooBarQuaxx opened this issue Dec 7, 2016 · 10 comments
Open

Set up auto-formatting of Python #1

FooBarQuaxx opened this issue Dec 7, 2016 · 10 comments

Comments

@FooBarQuaxx
Copy link

While exploring the code, I couldn't help but notice that the code style is not pep8 compliant. Following the standard way of writing code is very advantageous as it allow for gentle learning curve for someone how wants to contribute to the project.

@andychu
Copy link
Contributor

andychu commented Dec 7, 2016

The code is in the Google Python style, which is basically pep8 with 2 space indents and CapWords for function and method names. All the contributions will have to be in that style going forward, at least until the point where there's a significant amount of code not coming from me.

I changed this bug to be to set up auto-formatting. I have experimented with yapf but didn't apply it to all the code yet. I do want there to be a consistent style and for contributors to not have to worry about reformatting their code.

When the C++ code lands it will use clang-format as well.

@andychu andychu changed the title Non-standard code style Set up auto-formatting of Python and C++ Dec 7, 2016
@andychu
Copy link
Contributor

andychu commented Feb 4, 2018

Now running flake8, thanks @cclauss. It doesn't auto-format but it catches significant errors.

I'm still somewhat interested in yapf, but it's not a big priority.

@andychu andychu closed this as completed Feb 4, 2018
andychu pushed a commit that referenced this issue Mar 20, 2019
Case #1 in spec/prompt was newly failing.

- Also catch error for \]\[ (non_printing becomes negative, but ends up
  as 0)
- Make the prompt error messages more consistent
- Format to 80 cols and 2 space indent
@andychu andychu mentioned this issue Jul 1, 2019
@asokoloski
Copy link
Contributor

@andychu Hiya, I looked around for an Emacs mode for Google's python style, and couldn't find one anywhere. This seems to be the canonical source: https://github.com/google/styleguide

Based on that, it looks like Google doesn't use 2-space indentation for python anymore -- so they recommend using the default Emacs python mode style now. I don't know if that's enough reason to change the oilshell project style.

Assuming it's not, are you aware offhand of any Emacs mode files floating around for Google's old python style?

@oils-for-unix oils-for-unix deleted a comment from cclauss Oct 29, 2019
@andychu
Copy link
Contributor

andychu commented Oct 29, 2019

Sorry I'm not sure, I'm a vim user :-)

I will say that I accept any PR that passes tests :) The functionality is the more important part.

I may go back and update the style with subsequent commits. Or if there are consistent contributors then we can automate things more.

Right now flake8 runs in Travis, and I keep it green. It could be made more strict in the future.

@andychu
Copy link
Contributor

andychu commented Oct 29, 2019

Also this issue keeps getting closed because I type #1 in other contexts!

I'm reopening it although there a lot of other things that I think would benefit contributors more, e.g.

https://oilshell.zulipchat.com/#narrow/stream/121539-oil-dev/topic/Dev.20Dependencies.20Problem

@andychu andychu reopened this Oct 29, 2019
@asokoloski
Copy link
Contributor

Ok, thanks, no worries. I just didn't want to make a mess once I figure out a good way to contribute, but I'm sure I'll manage.

@cclauss
Copy link
Contributor

cclauss commented Oct 29, 2019

The Python Software Foundation is proposing the canonical way to format Python code and it is called black. It has integrations to Emacs, Vim, other editors. pre-commit hooks, and CI systems. Let's follow Google's lead for the formatting of Go code with gofmt but let's follow the PSF's lead for the formatting of Python code.

@andychu andychu changed the title Set up auto-formatting of Python and C++ Set up auto-formatting of Python Mar 21, 2020
@andychu
Copy link
Contributor

andychu commented Mar 21, 2020

test/lint.sh format-oil does this for C++

@cclauss
Copy link
Contributor

cclauss commented Mar 21, 2020

Oil bundles legacy Python which went end-of-life on 1/1/2020.
The black formatting tool for Python code will not run on Python 2.

@andychu
Copy link
Contributor

andychu commented Mar 21, 2020

Black can be installed by running pip install black. It requires Python 3.6.0+ to run but you can reformat Python 2 code with it, too.

https://pypi.org/project/black/

In any case this is a moot point until people who have contributed patches want it . It could be black or something else they prefer

andychu pushed a commit that referenced this issue Feb 6, 2021
Why didn't this happen on my own machine?

http://travis-ci.oilshell.org/srht-jobs/2021-02-06__19-34-07.wwz/_tmp/toil/logs/cpp-unit-all.txt

--> COLLECT with 0 roots
i = -2147483648
.AddressSanitizer:DEADLYSIGNAL
=================================================================
==3052==ERROR: AddressSanitizer: SEGV on unknown address 0x7f8666c53034 (pc 0x55618e7f6d5b bp 0x7ffc429f1190 sp 0x7ffc429f1180 T0)
==3052==The signal is caused by a READ memory access.
    #0 0x55618e7f6d5a in gc_heap::str_equals(gc_heap::Str*, gc_heap::Str*) /home/build/oil/mycpp/gc_heap.cc:260
    #1 0x55618e7dafd0 in str_replace_test /home/build/oil/mycpp/my_runtime_test.cc:162
    #2 0x55618e7e7bd6 in main /home/build/oil/mycpp/my_runtime_test.cc:779
    #3 0x7f866924109a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #4 0x55618e7d94b9 in _start (/home/build/oil/mycpp/_bin/my_runtime_test.asan+0xd4b9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/build/oil/mycpp/gc_heap.cc:260 in gc_heap::str_equals(gc_heap::Str*, gc_heap::Str*)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants