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

refactor: DB independent quoting and truthy/falsy values #31358

Merged
merged 11 commits into from
Jun 17, 2022
Merged

refactor: DB independent quoting and truthy/falsy values #31358

merged 11 commits into from
Jun 17, 2022

Conversation

c-p-b
Copy link
Contributor

@c-p-b c-p-b commented Jun 14, 2022

Minor, straightforward changes to db independent syntax for table quoting, spacing, and truthy/falsy coercion

@ankush ankush self-assigned this Jun 15, 2022
@ankush
Copy link
Member

ankush commented Jun 15, 2022

@cpdeethree can you once confirm the reason behind reformatting python strings to use single-quotes?

We now autoformat code using black, so it will be turned back to double quotes by black unnless it's considered unsafe like ' "double quotes inside" '. I don't think using single quote is a problem outside of SQL strings.

reference: https://github.com/frappe/erpnext/wiki/Pull-Request-Checklist#linting

@c-p-b
Copy link
Contributor Author

c-p-b commented Jun 16, 2022

As noted in our discussion, it looks like some of these changes happened when I originally went through and changed multiple double quoted values at once in the same file - even if it wasn't necessary. I will verify that there are no implications for postgres compatibility with the reformat you added (reverting back to the original)

@c-p-b
Copy link
Contributor Author

c-p-b commented Jun 16, 2022

Verified that PG has no problem with the changes you updated with the black commit 👍

@ankush ankush added the squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. label Jun 17, 2022
@@ -73,7 +73,7 @@ def get_stock_ledger_entries(report_filters):
"Stock Ledger Entry",
fields=fields,
filters=filters,
order_by="timestamp(posting_date, posting_time) asc, creation asc",
order_by="posting_date asc, posting_time asc, creation asc",
Copy link
Member

Choose a reason for hiding this comment

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

@cpdeethree I think this is fine

however in the past, people have suggested relying on the timestamp function instead of sorting columns individually, do you know of any real differences between these two?

ankush
ankush previously approved these changes Jun 17, 2022
@ankush ankush merged commit 74a782d into frappe:develop Jun 17, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accounts HR squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. stock
Projects
Development

Successfully merging this pull request may close these issues.

2 participants