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

[LiteX] Add support to pep8 / pylint for the "MiSoC / LiteX" formatting style & new specific checks #43

Open
mithro opened this issue Sep 10, 2017 · 2 comments

Comments

@mithro
Copy link
Member

mithro commented Sep 10, 2017

Brief explanation

Migen / LiteX doesn't quite follow pep8 / pylint for good reasons. Extend pep8 and/or pylint to understand when it should allow violations. Extra additional checks for good Migen / LiteX style should also be added.

Expected results

Running pep8-migen on Migen / LiteX HDL code does the following;

  • Checks all Python code that isn't Migen / LiteX complies with pep8 standard
  • Checks Migen / LiteX code compiles with the standard formatting style for Migen / LiteX
  • Checks for more common Migen / LiteX code smells.

Detailed Explanation

Due to the way the MiSoC / LiteX code works, this is preferred way to format the following statement;

    self.sync += [
        If(signal == 10,
            output1.eq(1),
        ).ElIf(signal == 5,
            output2.eq(0),
        ).Else(
            output3.eq(1),
        ),
    ]

See the migen documentation for more examples.

When the code is formatting this way it will fail pep8 with the following output;

$ pep8 misoc.py
misoc.py:6:9: E124 closing bracket does not match visual indentation
misoc.py:7:13: E128 continuation line under-indented for visual indent
misoc.py:8:9: E124 closing bracket does not match visual indentation

We need a pep8 / pylint extension which understand MiSoC / LiteX HDL formatting.

There are also a number of LiteX / Migen "code smells" that pep8 / pylint should also detect. Some are listed in the following section.

Migen / LiteX code smells

pep8 code smells which definitely apply

  • Using a bare try/except: clause
  • Pretty much most things
  • FIXME: Put more things here

pep8 code smells which do not apply

  • Wrapping of statements in pep8 style inside comb / sync blocks.
  • FIXME: Put more things here.

Incorrect wrapping of final brace

All lists / dictionaries should have commas on the last element;


No;

        self.comb += [
            counter0.eq(counter[0]),
            counter1.eq(counter[1]),
            counter2.eq(counter[2]),
            counter3.eq(counter[3]),
            ]

Yes;

        self.comb += [
            counter0.eq(counter[0]),
            counter1.eq(counter[1]),
            counter2.eq(counter[2]),
            counter3.eq(counter[3]),
        ]

Missing Comma on final element in list / dictionary

All lists / dictionaries should have commas on the last element;


No;

        self.comb += [
            counter0.eq(counter[0]),
            counter1.eq(counter[1]),
            counter2.eq(counter[2]),
            counter3.eq(counter[3])
        ]

Yes;

        self.comb += [
            counter0.eq(counter[0]),
            counter1.eq(counter[1]),
            counter2.eq(counter[2]),
            counter3.eq(counter[3]),
        ]

No;

        analyzer_signals = {
            0 : vcd_group,
            1 : sigrok_group
        }

Yes;

        analyzer_signals = {
            0 : vcd_group,
            1 : sigrok_group,
        }

Using line continuation rather than list

FIXME: Check this one makes sense...

No;

        self.sync.clk200 += \
            If(reset_counter != 0,
                reset_counter.eq(reset_counter - 1)
            ).Else(
                ic_reset.eq(0)
            )

Yes;

        self.sync.clk200 += [
            If(reset_counter != 0,
                reset_counter.eq(reset_counter - 1)
            ).Else(
                ic_reset.eq(0)
            ),
        ]

Using the wrong docstring style

Migen / LiteX use the "Numpy DocString style" for docstring comments.

See examples;

Code should not be using the "Google DocString Style" and not the "PEP 287 DocString Style"

This is supported by the napoleon module in Sphinx.

Not using yield from in test benches

https://m-labs.hk/migen/manual/simulation.html#pitfalls

When calling other testbenches, it is important to not forget the yield from. If it is omitted, the call would silently do nothing.

Further reading

Knowledge Prerequisites

  • List of
  • what knowledge is required
  • to complete this project.

Contacts

@mithro
Copy link
Member Author

mithro commented Sep 10, 2017

  • TODO: Find a good pylint configuration for Migen / LiteX? Maybe @m-labs has one?

@enjoy-digital
Copy link
Member

enjoy-digital commented Sep 10, 2017

The main contributors of migen/misoc are not using the following rules:

  • Missing Comma on final element in list / dictionary
  • Using line continuation rather than list

I see why you want that (to ease future changes in the code), but this should be an advice and probably not a rule. Some can found code easier to read without this so would avoid comma where not really needed or allow use of line continuation when only one element is in the comb/sync statement.

@mithro mithro changed the title Add support to pep8 / pylint for the "MiSoC / LiteX" formatting style & new specific checks [LiteX] Add support to pep8 / pylint for the "MiSoC / LiteX" formatting style & new specific checks Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants