Update database engine postgresql #280
Conversation
| ) | ||
| for key, val in zip(node.keys, node.values) | ||
| ) | ||
| and state.looks_like_settings_file() |
There was a problem hiding this comment.
we can go a bit faster by putting this settings file check first (and caching it: #281 ), since it's the most likely check to exclude a node from consideration
| yield ast_start_offset(target_node), partial( | ||
| replace, src='"django.db.backends.postgresql"' | ||
| ) |
There was a problem hiding this comment.
since #260, django-upgrade tries to match the quoting choice of existing code, so updating this to use a small token function to match the existing quote char
There was a problem hiding this comment.
ah yeah right, I've seen in the blog post you merge that in the end. mb
| settings, | ||
| filename="myapp/settings.py", | ||
| ) | ||
|
|
There was a problem hiding this comment.
We should also test when DATABASES has two relevant backends, I'm inserting a test here
| @@ -0,0 +1,52 @@ | |||
| """ | |||
There was a problem hiding this comment.
the name of the file was missing “t” 😉
| and isinstance(parents[-1], ast.Dict) | ||
| and isinstance((db_setting := parents[-2]), ast.Assign) | ||
| and isinstance(db_setting.targets[0], ast.Name) | ||
| and db_setting.targets[0].id == "DATABASES" | ||
| and len(db_setting.targets) == 1 |
There was a problem hiding this comment.
length checks should be done before accessing list indexes, to avoid crashes where the list isn't the required length. I don't think it's possible to have a crash here, though:
parents[-1]will always exist, and if it's aDict,parents[-2]will always exist, at least as theast.Module- I don't think it's possible for an
Assignto have 0 targets (would be weird)
But still worth putting the length checks first to embed the right pattern, plus they should be cheaper than the isinstance checks.
There was a problem hiding this comment.
I don't think it's possible for an Assign to have 0 targets (would be weird)
That's the assumption I made and the reason why I delayed the len check but yeah I'm not sure it's faster anyway and it's less clear so let's keep your version.
parents[-1] will always exist, and if it's a Dict, parents[-2] will always exist, at least as the ast.Module
yeah exactly, but I agree it might be confusing. Better explicit than implicit and the performance cost wont matter anyway
|
One small concern I just had - this will break code like: if settings.DATABASES["default"]["ENGINE"] == "django.db.backends.postgresql_psycopg2":
...and there’s not really much we can do to discover that (could be looked at in arbitrary variables) I think that’srather unlikely though, since the recommended way to check a DB vendor is: from django.db import connection
if connection.vendor == "postgres":
...WDYT? |
|
yeah it looks rather unlikely I agree. Also the first few stack overflow question i found about that were generally recommandation the Maybe you should ask the folks on twitter which one they would have used to get more feedback on that ? I feel like the recommended way is maybe not that well documented so I expect most people to land on stack overflow if they want to know how to check the project database vendor. |
Good research.
I think this is kinda niche, likely won't see many responses. Anyway we trust users to still be adults. If they use django-upgrade to change this string they’re responsible for any downstream effects. |
Another small one 😄.
Fixes #268