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.

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

Preserve double quotes when rewriting string tokens #260

Merged
merged 4 commits into from Oct 26, 2022

Conversation

kevinmarsh
Copy link
Contributor

I was attempting to use django-upgrade on a project which prefers double quotes over single quotes, meaning when something was rewritten like:

-request.META["HTTP_SERVER"]
+request.headers['Server']

it's linter would complain that it's expecting double quotes.

I'm not sure this solution is fully comprehensive but wanted to get feedback before spending any more time on it, but hopefully it's not too controversial to keep the existing quote style during rewrites.

@adamchainz
Copy link
Owner

Thanks for the report, but I do not want to pursue this route. As the readme says:

django-upgrade focuses on upgrading your code and not on making it look nice. Run django-upgrade before formatters like Black.

Pretty sure 99% of users will just run Black afterwards. Myabe you do too if you want double quoted strings. The pre-commit install instructions also tell you how to place django-upgrade to do this.

@adamchainz adamchainz closed this Sep 30, 2022
@adamchainz
Copy link
Owner

Please in future open an issue for discussion before doing any development.

Btw there is a good reason for using repr(), it handles adding escaping of special characters automatically. There's no double quoted version of it, so at best we could convert the repr, but it feels error prone and kinda pointless when code format gets already exist.

@kevinmarsh
Copy link
Contributor Author

No worries, I figured that'd be the case of not wanting to inspect the user's code more than you have to.

before doing any development

I created the PRs rather than issues since I still wanted to still use django-upgrade on that particular project to get it updated (done with a forked version) and just to show what the diff could look like, so my time wasn't wasted and hopefully yours wasn't too much...

@adamchainz adamchainz reopened this Oct 26, 2022
@adamchainz
Copy link
Owner

On second thought I think this is a good idea.

@adamchainz adamchainz enabled auto-merge (squash) October 26, 2022 09:24
@adamchainz adamchainz merged commit 3aa1579 into adamchainz:main Oct 26, 2022
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.

None yet

2 participants