Skip to content

Conversation

@gfrn
Copy link
Collaborator

@gfrn gfrn commented Mar 13, 2025

Since this is already being declared as an autogenerated ORM model, we do not need to declare it again. Doing so triggers a deprecation warning, and may cause undefined behaviour in future versions of SQLAlchemy.

This fixes #233

@codecov
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.20%. Comparing base (88e2378) to head (1d53513).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #234      +/-   ##
==========================================
- Coverage   92.20%   92.20%   -0.01%     
==========================================
  Files          27       27              
  Lines        4144     4143       -1     
  Branches      166      166              
==========================================
- Hits         3821     3820       -1     
  Misses        230      230              
  Partials       93       93              
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gfrn gfrn self-assigned this Mar 13, 2025
@gfrn gfrn requested a review from ndevenish March 13, 2025 10:46
@gfrn gfrn marked this pull request as draft March 13, 2025 14:13
@ndevenish
Copy link
Collaborator

Do the other relationship declarations here have the same issue?

@ndevenish
Copy link
Collaborator

Also, I'm assuming the reason that #227 originally errored without this was that 8da097b hadn't been run/merged in, and so fixing before that merge cause the redundant declaration. Maybe we should make orm-update just apply the regeneration in-PR.

@gfrn
Copy link
Collaborator Author

gfrn commented Mar 13, 2025

Do the other relationship declarations here have the same issue?

I didn't expect them to, since they had different names and my local tests didn't fail, at least until Jake mentioned it. I've now replaced the other declarations with synonyms

Also, I'm assuming the reason that #227 originally errored without this was that 8da097b hadn't been run/merged in, and so fixing before that merge cause the redundant declaration. Maybe we should make orm-update just apply the regeneration in-PR.

Agreed, this might only happen again with new SQLAlchemy versions (in the distant future), but this is a good idea

@gfrn gfrn marked this pull request as ready for review March 13, 2025 14:44
@ndevenish
Copy link
Collaborator

You happy to merge or did you want to check anything else?

@gfrn
Copy link
Collaborator Author

gfrn commented Mar 14, 2025

You happy to merge or did you want to check anything else?

Happy to merge it, I'll do it now

@gfrn gfrn merged commit 6d983d8 into main Mar 14, 2025
12 checks passed
@gfrn gfrn deleted the remove-manual-autoprocscalingstats-declaration branch March 14, 2025 08:37
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

Successfully merging this pull request may close these issues.

AutoProcScaling.AutoProcScalingStatistics in ispyb.sqlalchemy causes a deprecation warning

3 participants