iframe-proxy

ariebovenberg · 2022-01-19T20:35:28Z

Some __slots__ were not in order.

Fixes #7527

Description

I'm fixing two types of slots mistakes:

remove overlapping slots (i.e. slots already defined on a base class)
fix broken inheritance (i.e. slots class inheriting from a non-slots class)
- slots added to base class TransactionalContext. It seemed to use two attributes, which I've added as slots.
- empty slots removed from ORMOption. Its base class explicitly makes use of __dict__ so empty slots don't add anything.
- empty slots added to PostLoader. It doesn't appear to use any slots not already defined on its base classes.
- empty slots added to IterateMappersMixin. It doesn't appear to use any slots not already defined on its subclasses.
- empty slots added to ImmutableContainer. It doesn't use any fields.
- empty slots added to OperatorType. It's a protocol.
- empty slots added to InternalTraversal, _HasTraversalDispatch. They don't seem to use attributes on their own.

Checklist

This pull request is:

A short code fix
- please include the issue number, and create an issue if none exists, which
  must include a complete example of the issue. one line code fixes without an
  issue and demonstration will not be accepted.
- Please include: Fixes: #<issue number> in the commit message
- please include tests. one line code fixes without tests will not be accepted.

Have a nice day!

zzzeek · 2022-01-20T18:52:56Z

ariebovenberg · 2022-01-20T18:54:31Z

@zzzeek yes! I'd like to keep it in draft until all slots have been fixed, but I do need full CI runs to ensure nothing gets messed up 🙏

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision c5aacf4 of this pull request into gerrit so we can run tests and reviews and stuff

zzzeek · 2022-01-20T18:56:31Z

OK the github actions will run each time now. the gerrit review is a separate CI that I always invoke manually but the GH actions are enough for you to develop.

zzzeek · 2022-01-20T18:57:01Z

note I have some slots related changes coming through soon so that we can use slots more easily: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3532

sqla-tester · 2022-01-20T18:57:37Z

New Gerrit review created for change c5aacf4: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548

CaselIT · 2022-01-20T20:12:06Z

OK the github actions will run each time now. the gerrit review is a separate CI that I always invoke manually but the GH actions are enough for you to develop.

nope, only once we merge this. since it's a new contributor we have to approve each time.

@ariebovenberg ping us when you want the test to run

ariebovenberg · 2022-01-20T20:46:54Z

@CaselIT thanks for your patience -- I need some time to get familiar with the CI setup.

Am I correct in considering these failures/aborts unrelated?

CaselIT · 2022-01-20T20:57:41Z

The github ones are fine, so it's probably something unrelated.

Once you want to re-run the complete suite we can make to bot update the existing gerrit

zzzeek · 2022-01-20T21:52:57Z

I tried adding Python 3.11.0a4 to gerrit / jenkins today and it was a total failure. ignore anything on gerrit for now.

sqla-tester · 2022-01-20T23:06:30Z

mike bayer (zzzeek) wrote:

recheck

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548

ariebovenberg · 2022-01-21T16:16:06Z

Alright, the slots fixes seem complete from my end. I've edited the top comment with a list of fixed slots and reasoning behind each of them.

Questions for you:

You mention "Please include: Fixes: #<issue number> in the commit message". Should I squash the comments into one commit with this addition? Or add them to each commit?
Regarding tests: I would (of course 😄 ) suggest slotscheck, which I developed. I've recently added slotscheck to instagram/LibCST and dry-python/returns if you're curious what this looks like.

CaselIT · 2022-01-21T17:33:55Z

regarding 1, no, it's fine as multiple commits since it will be squashed when importing it in gerrit (the change will be merged by gerrit, not from github)

CaselIT · 2022-01-21T17:34:10Z

can we make this ready for review?

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 4f3f665 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-01-21T17:55:02Z

Patchset 4f3f665 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548

sqla-tester · 2022-01-30T14:14:12Z

mike bayer (zzzeek) wrote:

recheck

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548

sqla-tester · 2022-01-30T14:14:34Z

mike bayer (zzzeek) wrote:

we were having some CI issues a week ago which are fixed, not sure of the status of this so let's see if it passes CI

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548

ariebovenberg · 2022-01-30T15:51:50Z

Looks like some fresh issues that are likely caused by the slots change, likecannot create weak reference to 'RootTransaction' object. Will attempt a fix

ariebovenberg · 2022-01-31T21:10:43Z

@CaselIT could you kick off a CI run -- the weakref issue should be fixed. There may be more...

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision b6f4e8b of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-01-31T23:40:23Z

Patchset b6f4e8b added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548

ariebovenberg · 2022-02-01T19:57:52Z

I've rebased from main, which resulted in a few more slots issues.

@zzzeek I think I'm missing some of the background on orm.interfaces:ORMOption and sql.base:Options. These classes seem to explicitly use __dict__ in their methods, while __dict__ doesn't get created if slots are defined in the full MRO. It seems to me __slots__ = () can be removed from these classes.

Could you kick off another CI run?

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 70a9c4d of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-02-01T20:08:26Z

Patchset 70a9c4d added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548

sqla-tester · 2022-02-04T17:11:16Z

mike bayer (zzzeek) wrote:

this looks great! did you say that you have a tool that lints for these? if it's mature enough we can have it as part of our .pre-commit config, if it can just alert us to new slots changes that are overlapping or missing.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548

sqla-tester · 2022-02-04T17:11:29Z

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3548 has been merged. Congratulations! :)

ariebovenberg · 2022-02-05T20:20:59Z

As discussed in #7589, `slotscheck` can prevent slots-related mistakes from creeping back in. Plan for now is to have slotscheck part of the "lint" tests (renamed from pep8) that will run for CI and github actions. To support slotscheck's runtime nature, slotscheck is run twice, first with cython exts enabled and then with them disabled via new environment variable. Also added sqlalchemy[mypy] dependency to support slots checking the mypy plugin. Found and fixed one more `__slots__` issue by disabling C exts. Closes: #7670 Pull-request: #7670 Pull-request-sha: 3e77fe5 Change-Id: I90cdd284cdcee316a38856ba94d72ffc98947c5a

Some `__slots__` were not in order. Fixes sqlalchemy#7527 ### Description I'm fixing two types of slots mistakes: - [x] remove overlapping slots (i.e. slots already defined on a base class) - [x] fix broken inheritance (i.e. slots class inheriting from a non-slots class) - [x] slots added to base class `TransactionalContext`. It seemed to use two attributes, which I've added as slots. - [x] empty slots removed from `ORMOption`. Its base class explicitly makes use of `__dict__` so empty slots don't add anything. - [x] empty slots added to `PostLoader`. It doesn't appear to use any slots not already defined on its base classes. - [x] empty slots added to `IterateMappersMixin`. It doesn't appear to use any slots not already defined on its subclasses. - [x] empty slots added to `ImmutableContainer`. It doesn't use any fields. - [x] empty slots added to `OperatorType`. It's a protocol. - [x] empty slots added to `InternalTraversal`, `_HasTraversalDispatch`. They don't seem to use attributes on their own. ### Checklist This pull request is: - [x] A short code fix - please include the issue number, and create an issue if none exists, which must include a complete example of the issue. one line code fixes without an issue and demonstration will not be accepted. - Please include: `Fixes: #<issue number>` in the commit message - please include tests. one line code fixes without tests will not be accepted. **Have a nice day!** Closes: sqlalchemy#7589 Pull-request: sqlalchemy#7589 Pull-request-sha: 70a9c4d Change-Id: I6c6e3e69c3c34d0f3bdda7f0684849834fdd1863

As discussed in sqlalchemy#7589, `slotscheck` can prevent slots-related mistakes from creeping back in. Plan for now is to have slotscheck part of the "lint" tests (renamed from pep8) that will run for CI and github actions. To support slotscheck's runtime nature, slotscheck is run twice, first with cython exts enabled and then with them disabled via new environment variable. Also added sqlalchemy[mypy] dependency to support slots checking the mypy plugin. Found and fixed one more `__slots__` issue by disabling C exts. Closes: sqlalchemy#7670 Pull-request: sqlalchemy#7670 Pull-request-sha: 3e77fe5 Change-Id: I90cdd284cdcee316a38856ba94d72ffc98947c5a

ariebovenberg changed the title ~~remove overlapping slots already defined in base classes~~ Fix overlapping slots, base classes without slots Jan 20, 2022

zzzeek requested a review from sqla-tester January 20, 2022 18:55

sqla-tester reviewed Jan 20, 2022

View reviewed changes

ariebovenberg marked this pull request as ready for review January 21, 2022 17:45

CaselIT requested a review from sqla-tester January 21, 2022 17:54

sqla-tester reviewed Jan 21, 2022

View reviewed changes

CaselIT requested a review from sqla-tester January 31, 2022 23:40

sqla-tester reviewed Jan 31, 2022

View reviewed changes

ariebovenberg added 3 commits February 1, 2022 20:26

remove overlapping slots already defined in base classes

bf7defc

add slots to base class TransactionalContext

f32b2cd

remove unused slots from ORMOption, add empty slots to PostLoader

dc5e0cb

ariebovenberg added 5 commits February 1, 2022 20:26

add empty slots to two base classes

884d795

add empty slots to three more base classes

e448f97

allow weakref to TransactionContext classes

4d69a69

make sure _trans_subject in __exit__ always has a value

eb8a475

add missing slots to three more classes

70a9c4d

ariebovenberg force-pushed the fix-slots branch from b6f4e8b to 70a9c4d Compare February 1, 2022 19:52

CaselIT requested a review from sqla-tester February 1, 2022 20:08

sqla-tester reviewed Feb 1, 2022

View reviewed changes

sqlalchemy-bot closed this in 0409c88 Feb 4, 2022

ariebovenberg deleted the fix-slots branch February 4, 2022 19:13

ariebovenberg mentioned this pull request Feb 5, 2022

add slotscheck to CI #7670

Closed

1 task

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

Conversation

ariebovenberg commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

zzzeek commented Jan 20, 2022

Uh oh!

ariebovenberg commented Jan 20, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

zzzeek commented Jan 20, 2022

Uh oh!

zzzeek commented Jan 20, 2022

Uh oh!

sqla-tester commented Jan 20, 2022

Uh oh!

CaselIT commented Jan 20, 2022

Uh oh!

ariebovenberg commented Jan 20, 2022

Uh oh!

CaselIT commented Jan 20, 2022

Uh oh!

zzzeek commented Jan 20, 2022

Uh oh!

sqla-tester commented Jan 20, 2022

Uh oh!

ariebovenberg commented Jan 21, 2022

Uh oh!

CaselIT commented Jan 21, 2022

Uh oh!

CaselIT commented Jan 21, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Jan 21, 2022

Uh oh!

sqla-tester commented Jan 30, 2022

Uh oh!

sqla-tester commented Jan 30, 2022

Uh oh!

ariebovenberg commented Jan 30, 2022

Uh oh!

ariebovenberg commented Jan 31, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Jan 31, 2022

Uh oh!

ariebovenberg commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Feb 1, 2022

Uh oh!

sqla-tester commented Feb 4, 2022

Uh oh!

sqla-tester commented Feb 4, 2022

Uh oh!

ariebovenberg commented Feb 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ariebovenberg commented Jan 19, 2022 •

edited

Loading

ariebovenberg commented Feb 1, 2022 •

edited

Loading