Fix overlapping, missing slots

[ariebovenberg]

There are some misconfigured __slots__:

$ slotscheck -m piccolo
ERROR: 'piccolo.columns.combination:Combination' has slots but superclass does not.
ERROR: 'piccolo.columns.combination:Where' has slots but superclass does not.
ERROR: 'piccolo.query.methods.alter:AddColumn' defines overlapping slots.
ERROR: 'piccolo.query.methods.alter:SetDefault' defines overlapping slots.
Oh no, found some problems!
Scanned 145 module(s), 261 class(es).

Thankfully the fix appears easy.

I discovered the slots issues with slotscheck, a tool I maintain. Of course, If you like, I can add it to CI as I've done for instagram/LibCST, sqlalchemy/sqlalchemy, and aio-libs/aiohttp.

[codecov-commenter]

Codecov Report

Merging #426 (b97a653) into master (2d70ba9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #426   +/-   ##
=======================================
  Coverage   90.84%   90.84%           
=======================================
  Files         103      103           
  Lines        6868     6870    +2     
=======================================
+ Hits         6239     6241    +2     
  Misses        629      629           

Impacted Files	Coverage Δ
piccolo/columns/combination.py	94.66% <100.00%> (+0.14%)	⬆️
piccolo/query/methods/alter.py	89.31% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d70ba9...b97a653. Read the comment docs.

[dantownsend]

@ariebovenberg This is great, thanks.

Slots are tricky to work with, and error prone, as your tool illustrates. slotscheck is impressively fast.

I think it's worthwhile adding slotscheck to Piccolo's test suite. If you're happy to do it, it just needs adding here and here.

One feature you could potentially add in the future is emitting a warning for this situation:

class Animal:
    __slots__ = ('colour',)

class Dog(Animal):
    pass  # Because it doesn't define slots, Python still assigns a dictionary, should be `__slots__ = ()`

This might already be supported, and I'm not aware.

[ariebovenberg]

I added it to the lint script, but chose not to run it on tests/. This because tests are probably not meant to be imported/run outside of test context. slotscheck does support ignoring certain modules, but I felt excluding tests altogether was probably best. Let me know if you'd like to include it anyway, it should be easy to add an ignore for specific test modules that cannot be imported.

Regarding your idea about warnings: I'm very interested to hear any thoughts about slots 'enforcement' policies! Slotscheck does have a superstrict require-subclass mode, which does what you describe, but also counts object as a slotted class. Perhaps a 'softer' version of this rule could be useful 🤔 . I've opened ariebovenberg/slotscheck#59 in case you have any thoughts about this.

[dantownsend]

@ariebovenberg Thanks for this. I agree, there's no need to check the tests/ folder.

I left a comment on the issue you opened.

[dantownsend]

@ariebovenberg The only other suggestion I have for slotscheck is it feels to me like this should work:

slotscheck piccolo

But instead it's:

python -m slotscheck piccolo

Which is fine, but I think it would be more intuitive if both worked.

[ariebovenberg]

@dantownsend indeed that can be confusing. This behavior is determined by Python itself, so I'm hesitant to override it. This is the approach also followed by pyanalyze, so slotscheck is in good company.

There's a detailed description in the docs about this potentially confusing behavior. Now that I think about it...it'd probably be a good idea to add a link to this whenever a module cannot be found that corresponds to a directory in the working directory 🤔 — to prevent any unnecessary frustration (ariebovenberg/slotscheck#60)

[dantownsend]

@ariebovenberg Interesting - it makes a lot more sense now I've read your docs about it.

I can see your reasoning for doing it that way.