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.

Sign up for GitHub

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

Jump to bottom

DM-41577: squash and merge eac-dev to main #7

Merged
merged 5 commits into from
Nov 4, 2023
Merged

DM-41577: squash and merge eac-dev to main #7

merged 5 commits into from
Nov 4, 2023

Conversation

fritzm
Copy link
Member

@fritzm fritzm commented Nov 4, 2023

No description provided.

@fritzm
Update dependencies
d773d65
@fritzm
Eric's Big Adventure
c1dc3e7 
This commit addresses the greater part of Eric's v1 feature-parity work,
formerly the `eac-dev` branch.

This commit has not yet been DM reviewed, but does pass linters, is mypy
clean, and has a functional test suite -- in light of this, the team has
chosen to conduct further review prep work incrementally on `main`.
@eacharles
Copy link
Collaborator

This is the best code I've ever seen. Surely it sprung fully formed, sun generis, from the blood of dying gods.

We must merge this immediately to bring an end to the dark ages and cast off the shackles of our oppression.

eacharles
eacharles approved these changes Nov 4, 2023
View reviewed changes
Copy link
Collaborator

@eacharles eacharles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extraordinary code should be merged immediately to bring light unto the masses that they may rejoice.

@fritzm fritzm force-pushed the tickets/DM-41577 branch 3 times, most recently from 31bc7dc to 5a15b7b Compare November 4, 2023 19:47
@eacharles
Copy link
Collaborator

Cool that each linter gots its own vision, looking forward to meta linter!!

@fritzm fritzm force-pushed the tickets/DM-41577 branch 4 times, most recently from 0f3bce3 to b843bca Compare November 4, 2023 20:55
@eacharles
Copy link
Collaborator

eacharles commented Nov 4, 2023 •
edited
Loading

Looks like the tickets/DM-41577 branch is being pushed around..

@fritzm
Copy link
Member Author

fritzm commented Nov 4, 2023

Okay, I think I'm done fiddling with the PR now.

  • Had a go with ruff ALL, and it found a lot of interesting stuff, some of which I think is actually substantial and which we'll want to discuss/ticket/fix after the merge.

  • Let ruff do a bunch of trivial auto-fixes, and hand implemented a few more of its suggestions which I thought would be non-controversial. These are on one commit and probably aren't review-worthy, other than having a peek here and there if you wish to see the sorts of things that were changed.

  • Fixed the warning re. deprecated FastAPI lifetime event handlers by just upgrading to the latest new hotness there.

  • Found a way to address the annoying lint with name shadowing in pytest fixtures, so was able to remove # pylint: disables there.

  • There is something really unsavory still happening with session management in the test suite, which you can see it whinging about when you make test... I stared hard at it for a while, but didn't find an immediate fix. We'll probably want to return to work on this soon also

@fritzm fritzm merged commit 05a9a04 into main Nov 4, 2023
7 checks passed
@fritzm fritzm deleted the tickets/DM-41577 branch November 4, 2023 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eacharles eacharles eacharles approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fritzm @eacharles