Sourcery refactored develop branch #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
| # if there is already an entry in this account then just add it | ||
| # to that entry | ||
| same_head = check_if_in_list(entry, merged_gl_map) | ||
| if same_head: | ||
| if same_head := check_if_in_list(entry, merged_gl_map): |
There was a problem hiding this comment.
Function merge_similar_entries refactored with the following changes:
- Use named expression to simplify assignment and conditional (
use-named-expression)
This removes the following comments ( why? ):
# to that entry
# if there is already an entry in this account then just add it
Comment on lines
-50
to
+54
| billing_address_field = "customer_address" if party_type == "Lead" \ | ||
| else party_type.lower() + "_address" | ||
| billing_address_field = ( | ||
| "customer_address" | ||
| if party_type == "Lead" | ||
| else f"{party_type.lower()}_address" | ||
| ) |
There was a problem hiding this comment.
Function set_address_details refactored with the following changes:
- Use f-string instead of string concatenation (
use-fstring-for-concatenation)
Comment on lines
-85
to
+89
| if party.get("default_" + f): | ||
| out[f] = party.get("default_" + f) | ||
| if party.get(f"default_{f}"): | ||
| out[f] = party.get(f"default_{f}") |
There was a problem hiding this comment.
Function set_other_values refactored with the following changes:
- Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation)
Comment on lines
-124
to
-129
| out = { | ||
| return { | ||
| party_type.lower(): party, | ||
| account_fieldname : account, | ||
| "due_date": get_due_date(posting_date, party, party_type, account, company) | ||
| account_fieldname: account, | ||
| "due_date": get_due_date( | ||
| posting_date, party, party_type, account, company | ||
| ), | ||
| } | ||
| return out |
There was a problem hiding this comment.
Function set_account_and_due_date refactored with the following changes:
- Inline variable that is immediately returned (
inline-immediately-returned-variable)
Comment on lines
-166
to
+172
| if not frappe.db.exists("Account", (party.strip() + " - " + company_details.abbr)): | ||
| if not frappe.db.exists( | ||
| "Account", f"{party.strip()} - {company_details.abbr}" | ||
| ): |
There was a problem hiding this comment.
Function create_party_account refactored with the following changes:
- Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation)
Comment on lines
-16
to
+20
| condition = "" | ||
| if not self.include_reconciled_entries: | ||
| condition = "and ifnull(clearance_date, '') in ('', '0000-00-00')" | ||
|
|
||
|
|
||
| condition = ( | ||
| "" | ||
| if self.include_reconciled_entries | ||
| else "and ifnull(clearance_date, '') in ('', '0000-00-00')" | ||
| ) |
There was a problem hiding this comment.
Function BankReconciliation.get_details refactored with the following changes:
- Move setting of default value for variable into
elsebranch (introduce-default-else) - Swap if/else branches of if expression to remove negation (
swap-if-expression) - Replace if statement with if expression (
assign-if-exp)
Comment on lines
-14
to
-19
| idx =1 | ||
| for m in month_list: | ||
| for idx, m in enumerate(month_list, start=1): | ||
| mnth = self.append('budget_distribution_details') | ||
| mnth.month = m | ||
| mnth.idx = idx | ||
| idx += 1 |
There was a problem hiding this comment.
Function BudgetDistribution.get_months refactored with the following changes:
- Replace manual loop counter with call to enumerate (
convert-to-enumerate)
Comment on lines
-22
to
+23
| total = sum([flt(d.percentage_allocation) for d in self.get("budget_distribution_details")]) | ||
| total = sum( | ||
| flt(d.percentage_allocation) | ||
| for d in self.get("budget_distribution_details") | ||
| ) |
There was a problem hiding this comment.
Function BudgetDistribution.validate refactored with the following changes:
- Replace unneeded comprehension with generator (
comprehension-to-generator)
| frappe.throw("C-form is not applicable for Invoice: %s" % d.invoice_no) | ||
| frappe.throw(f"C-form is not applicable for Invoice: {d.invoice_no}") |
There was a problem hiding this comment.
Function CForm.validate refactored with the following changes:
- Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring)
Comment on lines
-45
to
+49
| inv = [d.invoice_no for d in self.get('invoice_details')] | ||
| if inv: | ||
| frappe.db.sql("""update `tabSales Invoice` set c_form_no=%s, modified=%s where name in (%s)""" % | ||
| ('%s', '%s', ', '.join(['%s'] * len(inv))), tuple([self.name, self.modified] + inv)) | ||
| if inv := [d.invoice_no for d in self.get('invoice_details')]: | ||
| frappe.db.sql( | ||
| f"""update `tabSales Invoice` set c_form_no=%s, modified=%s where name in ({', '.join(['%s'] * len(inv))})""", | ||
| tuple([self.name, self.modified] + inv), | ||
| ) |
There was a problem hiding this comment.
Function CForm.set_cform_in_sales_invoices refactored with the following changes:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring) - Simplify unnecessary nesting, casting and constant values in f-strings (
simplify-fstring-formatting)
Comment on lines
-57
to
+58
| total = sum([flt(d.grand_total) for d in self.get('invoice_details')]) | ||
| total = sum(flt(d.grand_total) for d in self.get('invoice_details')) |
There was a problem hiding this comment.
Function CForm.set_total_invoiced_amount refactored with the following changes:
- Replace unneeded comprehension with generator (
comprehension-to-generator)
Comment on lines
-17
to
+33
|
|
||
| from erpnext.accounts.doctype.chart_of_accounts.charts.account_properties import account_properties | ||
|
|
||
| if chart: | ||
| accounts = [] | ||
|
|
||
| def _import_accounts(children, parent): | ||
| for child in children: | ||
| account_name = child.get("name") | ||
| account_name_in_db = unidecode(account_name.strip().lower()) | ||
|
|
||
| if account_name_in_db in accounts: | ||
| count = accounts.count(account_name_in_db) | ||
| account_name = account_name + " " + cstr(count) | ||
| account_name = f"{account_name} {cstr(count)}" | ||
|
|
||
| child.update(account_properties.get(chart.get("name"), {}).get(account_name, {})) | ||
|
|
There was a problem hiding this comment.
Function ChartofAccounts.create_accounts refactored with the following changes:
- Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation)
Comment on lines
-56
to
+61
| default_types_root = [] | ||
| for file in ["data_account_type.xml"]: | ||
| default_types_root.append(ET.parse(os.path.join(path, "account", "data", | ||
| "data_account_type.xml")).getroot()) | ||
| default_types_root = [ | ||
| ET.parse( | ||
| os.path.join(path, "account", "data", "data_account_type.xml") | ||
| ).getroot() | ||
| for _ in ["data_account_type.xml"] | ||
| ] |
There was a problem hiding this comment.
Function get_default_account_types refactored with the following changes:
- Convert for loop into list comprehension (
list-comprehension) - Replace unused for index with underscore (
for-index-underscore)
Comment on lines
-125
to
+130
| node_id = prefix + "." + node.get("id") if prefix else node.get("id") | ||
|
|
||
| node_id = f"{prefix}." + node.get("id") if prefix else node.get("id") | ||
| types[node_id] = data | ||
|
|
There was a problem hiding this comment.
Function get_account_types refactored with the following changes:
- Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation)
Comment on lines
-15
to
+17
| self.name = self.cost_center_name.strip() + ' - ' + \ | ||
| frappe.db.get_value("Company", self.company, "abbr") | ||
| self.name = f'{self.cost_center_name.strip()} - ' + frappe.db.get_value( | ||
| "Company", self.company, "abbr" | ||
| ) |
There was a problem hiding this comment.
Function CostCenter.autoname refactored with the following changes:
- Use f-string instead of string concatenation (
use-fstring-for-concatenation)
Comment on lines
-13
to
+21
| import test_records as jv_test_records | ||
| import test_records as jv_test_records | ||
| from erpnext.selling.doctype.sales_order.test_sales_order \ | ||
| import test_records as so_test_records | ||
| import test_records as so_test_records | ||
| from erpnext.buying.doctype.purchase_order.test_purchase_order \ | ||
| import test_records as po_test_records | ||
| import test_records as po_test_records | ||
| from erpnext.accounts.doctype.sales_invoice.test_sales_invoice \ | ||
| import test_records as si_test_records | ||
| import test_records as si_test_records | ||
| from erpnext.accounts.doctype.purchase_invoice.test_purchase_invoice \ | ||
| import test_records as pi_test_records | ||
| import test_records as pi_test_records |
There was a problem hiding this comment.
Function TestPaymentTool.test_make_journal_voucher refactored with the following changes:
- Merge dictionary updates via the union operator (
dict-assign-update-to-union)
Comment on lines
-18
to
+22
| res = frappe.db.sql("""select name, user from `tabPOS Setting` | ||
| if res := frappe.db.sql( | ||
| """select name, user from `tabPOS Setting` | ||
| where ifnull(user, '') = %s and name != %s and company = %s""", | ||
| (self.user, self.name, self.company)) | ||
| if res: | ||
| (self.user, self.name, self.company), | ||
| ): |
There was a problem hiding this comment.
Function POSSetting.check_for_duplicate refactored with the following changes:
- Use named expression to simplify assignment and conditional (
use-named-expression)
Comment on lines
-54
to
+59
| if not include_current_pos: | ||
| condition = " where name != '%s'" % self.name.replace("'", "\'") | ||
| else: | ||
| condition = "" | ||
|
|
||
| condition = ( | ||
| "" | ||
| if include_current_pos | ||
| else " where name != '%s'" % self.name.replace("'", "\'") | ||
| ) |
There was a problem hiding this comment.
Function POSSetting.set_defaults refactored with the following changes:
- Replace if statement with if expression (
assign-if-exp) - Swap if/else branches of if expression to remove negation (
swap-if-expression)
Comment on lines
-134
to
+147
| if not args.item_group: | ||
| frappe.throw(_("Item Group not mentioned in item master for item {0}").format(args.item_code)) | ||
| if not args.item_group: | ||
| frappe.throw(_("Item Group not mentioned in item master for item {0}").format(args.item_code)) | ||
|
|
||
| if args.customer and not (args.customer_group and args.territory): | ||
| customer = frappe.db.get_value("Customer", args.customer, ["customer_group", "territory"]) | ||
| if customer: | ||
| if customer := frappe.db.get_value( | ||
| "Customer", args.customer, ["customer_group", "territory"] | ||
| ): | ||
| args.customer_group, args.territory = customer | ||
|
|
||
| elif args.supplier and not args.supplier_type: | ||
| args.supplier_type = frappe.db.get_value("Supplier", args.supplier, "supplier_type") | ||
|
|
||
| pricing_rules = get_pricing_rules(args) | ||
| pricing_rule = filter_pricing_rules(args, pricing_rules) | ||
|
|
||
| if pricing_rule: | ||
| if pricing_rule := filter_pricing_rules(args, pricing_rules): |
There was a problem hiding this comment.
Function get_pricing_rule_for_item refactored with the following changes:
- Hoist conditional out of nested conditional (
hoist-if-from-if) - Use named expression to simplify assignment and conditional [×2] (
use-named-expression)
Comment on lines
-172
to
+191
| condition = " ifnull("+field+", '') in ('" + \ | ||
| "', '".join([d.replace("'", "\\'").replace('"', '\\"') for d in parent_groups])+"')" | ||
| condition = ( | ||
| f" ifnull({field}, '') in ('" | ||
| + "', '".join( | ||
| [d.replace("'", "\\'").replace('"', '\\"') for d in parent_groups] | ||
| ) | ||
| ) + "')" | ||
| return condition | ||
|
|
||
|
|
||
|
|
||
| conditions = "" | ||
| for field in ["company", "customer", "supplier", "supplier_type", "campaign", "sales_partner"]: | ||
| if args.get(field): | ||
| conditions += " and ifnull("+field+", '') in (%("+field+")s, '')" | ||
| conditions += f" and ifnull({field}, '') in (%({field})s, '')" | ||
| else: | ||
| conditions += " and ifnull("+field+", '') = ''" | ||
| conditions += f" and ifnull({field}, '') = ''" | ||
|
|
||
| for parenttype in ["Customer Group", "Territory"]: | ||
| group_condition = _get_tree_conditions(parenttype) | ||
| if group_condition: | ||
| conditions += " and " + group_condition | ||
| conditions += f" and {group_condition}" |
There was a problem hiding this comment.
Function get_pricing_rules refactored with the following changes:
- Use f-string instead of string concatenation [×10] (
use-fstring-for-concatenation)
Comment on lines
-214
to
+219
| max_priority = max([cint(p.priority) for p in pricing_rules]) | ||
| if max_priority: | ||
| if max_priority := max(cint(p.priority) for p in pricing_rules): |
There was a problem hiding this comment.
Function filter_pricing_rules refactored with the following changes:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Replace list(), dict() or set() with comprehension (
collection-builtin-to-comprehension) - Replace unneeded comprehension with generator [×2] (
comprehension-to-generator)
Comment on lines
-244
to
+249
| all_rules_same = True | ||
| val = [pricing_rules[0][k] for k in fields] | ||
| for p in pricing_rules[1:]: | ||
| if val != [p[k] for k in fields]: | ||
| all_rules_same = False | ||
| break | ||
|
|
||
| return all_rules_same | ||
| return all(val == [p[k] for k in fields] for p in pricing_rules[1:]) |
There was a problem hiding this comment.
Function if_all_rules_same refactored with the following changes:
- Move assignment closer to its usage within a block (
move-assign-in-block) - Use any() instead of for loop (
use-any) - Invert any/all to simplify comparisons (
invert-any-all) - Inline variable that is immediately returned (
inline-immediately-returned-variable)
Comment on lines
-87
to
+91
| if d.item_code: # extra condn coz item_code is not mandatory in PV | ||
| if frappe.db.get_value("Item", d.item_code, "is_purchase_item") != 'Yes': | ||
| msgprint(_("Item {0} is not Purchase Item").format(d.item_code), raise_exception=True) | ||
| if ( | ||
| d.item_code | ||
| and frappe.db.get_value("Item", d.item_code, "is_purchase_item") != 'Yes' | ||
| ): | ||
| msgprint(_("Item {0} is not Purchase Item").format(d.item_code), raise_exception=True) |
There was a problem hiding this comment.
Function PurchaseInvoice.check_active_purchase_items refactored with the following changes:
- Merge nested if conditions (
merge-nested-ifs)
This removes the following comments ( why? ):
# extra condn coz item_code is not mandatory in PV
Comment on lines
-108
to
+114
| if (acc_head and cstr(acc_head[0][0]) != cstr(self.supplier)) or (not acc_head and (self.credit_to != cstr(self.supplier) + " - " + self.company_abbr)): | ||
| if ( | ||
| (acc_head and cstr(acc_head[0][0]) != cstr(self.supplier)) | ||
| or not acc_head | ||
| and self.credit_to != f"{cstr(self.supplier)} - {self.company_abbr}" | ||
| ): |
There was a problem hiding this comment.
Function PurchaseInvoice.check_for_acc_head_of_supplier refactored with the following changes:
- Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation)
Comment on lines
-116
to
+131
| if d.purchase_order and not d.purchase_order in check_list and not d.purchase_receipt: | ||
| if ( | ||
| d.purchase_order | ||
| and d.purchase_order not in check_list | ||
| and not d.purchase_receipt | ||
| ): | ||
| check_list.append(d.purchase_order) | ||
| stopped = frappe.db.sql("select name from `tabPurchase Order` where status = 'Stopped' and name = %s", d.purchase_order) | ||
| if stopped: | ||
| if stopped := frappe.db.sql( | ||
| "select name from `tabPurchase Order` where status = 'Stopped' and name = %s", | ||
| d.purchase_order, | ||
| ): |
There was a problem hiding this comment.
Function PurchaseInvoice.check_for_stopped_status refactored with the following changes:
- Simplify logical expression using De Morgan identities (
de-morgan) - Use named expression to simplify assignment and conditional (
use-named-expression)
Comment on lines
-245
to
+247
| for period in period_list: | ||
| columns.append({ | ||
| columns.extend( | ||
| { | ||
| "fieldname": period.key, | ||
| "label": period.label, | ||
| "fieldtype": "Currency", | ||
| "width": 150 | ||
| }) | ||
|
|
||
| "width": 150, | ||
| } | ||
| for period in period_list | ||
| ) |
There was a problem hiding this comment.
Function get_columns refactored with the following changes:
- Replace a for append loop with list extend (
for-append-to-extend)
Comment on lines
-15
to
+24
| account_map = dict(((r.name, r) for r in frappe.db.sql("""select acc.name, | ||
| account_map = { | ||
| r.name: r | ||
| for r in frappe.db.sql( | ||
| """select acc.name, | ||
| supp.supplier_name, supp.name as supplier | ||
| from `tabAccount` acc, `tabSupplier` supp | ||
| where acc.master_type="Supplier" and supp.name=acc.master_name""", as_dict=1))) | ||
| where acc.master_type="Supplier" and supp.name=acc.master_name""", | ||
| as_dict=1, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Function execute refactored with the following changes:
- Replace list(), dict() or set() with comprehension (
collection-builtin-to-comprehension) - Replace index in for loop with direct reference (
for-index-replacement) - Replace range(0, x) with range(x) (
remove-zero-from-range) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring)
Comment on lines
-89
to
+103
| gl_entries = frappe.db.sql("""select * from `tabGL Entry` | ||
| where docstatus < 2 %s order by posting_date, account""" % | ||
| (conditions), tuple(supplier_accounts), as_dict=1) | ||
| return gl_entries | ||
| return frappe.db.sql( | ||
| """select * from `tabGL Entry` | ||
| where docstatus < 2 %s order by posting_date, account""" | ||
| % (conditions), | ||
| tuple(supplier_accounts), | ||
| as_dict=1, | ||
| ) |
There was a problem hiding this comment.
Function get_gl_entries refactored with the following changes:
- Inline variable that is immediately returned (
inline-immediately-returned-variable)
Comment on lines
-98
to
+109
|
|
There was a problem hiding this comment.
Function get_conditions refactored with the following changes:
- Replace interpolated string formatting with f-string [×3] (
replace-interpolation-with-fstring)
Comment on lines
-121
to
+140
| account_supplier_type_map = {} | ||
| for each in frappe.db.sql("""select acc.name, supp.supplier_type from `tabSupplier` supp, | ||
| `tabAccount` acc where supp.name = acc.master_name group by acc.name"""): | ||
| account_supplier_type_map[each[0]] = each[1] | ||
|
|
||
| return account_supplier_type_map | ||
| return { | ||
| each[0]: each[1] | ||
| for each in frappe.db.sql( | ||
| """select acc.name, supp.supplier_type from `tabSupplier` supp, | ||
| `tabAccount` acc where supp.name = acc.master_name group by acc.name""" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Function get_account_supplier_type_map refactored with the following changes:
- Convert for loop into dictionary comprehension (
dict-comprehension) - Inline variable that is immediately returned (
inline-immediately-returned-variable)
Sourcery Code Quality Report❌ Merging this PR will decrease code quality in the affected files by 0.04%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Branch
developrefactored by Sourcery.If you're happy with these changes, merge this Pull Request using the Squash and merge strategy.
See our documentation here.
Run Sourcery locally
Reduce the feedback loop during development by using the Sourcery editor plugin:
Review changes via command line
To manually merge these changes, make sure you're on the
developbranch, then run:Help us improve this pull request!