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

perf: use get_cached_value instead of db.get_value in accounts module #32836

Conversation

DaizyModi
Copy link
Contributor

@DaizyModi DaizyModi commented Nov 3, 2022

Similar to #32776

Applied on Accounts Module

Before

In [1]: from erpnext.accounts.doctype.fiscal_year.fiscal_year import get_from_and_to_date
In [2]: %timeit get_from_and_to_date('2022-2023')
2.52 ms ± 63.4 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

After

In [1]: from erpnext.accounts.doctype.fiscal_year.fiscal_year import get_from_and_to_date
In [2]: %timeit get_from_and_to_date('2022-2023')
5.84 µs ± 203 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

@github-actions github-actions bot added accounts needs-tests This PR needs automated unit-tests. labels Nov 3, 2022
@deepeshgarg007
Copy link
Member

The changes look okay, anything else pending? @DaizyModi
Can we mark this as ready for review and merge?

@DaizyModi
Copy link
Contributor Author

The changes look okay, anything else pending? @DaizyModi Can we mark this as ready for review and merge?

A few changes are pending. I will open this PR by tomorrow.

@sagarvora sagarvora force-pushed the perf-fix-get-cached-value-for-accounts branch from 73fd04e to 8ae58ed Compare November 17, 2022 20:43
@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #32836 (62c0210) into develop (d1f85dd) will increase coverage by 0.00%.
The diff coverage is 58.94%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #32836   +/-   ##
========================================
  Coverage    63.83%   63.83%           
========================================
  Files          820      820           
  Lines        58766    58766           
========================================
+ Hits         37511    37512    +1     
+ Misses       21255    21254    -1     
Impacted Files Coverage Δ
...count_balance_timeline/account_balance_timeline.py 0.00% <0.00%> (ø)
...next/accounts/doctype/bank_account/bank_account.py 77.27% <0.00%> (ø)
...octype/bank_transaction/bank_transaction_upload.py 0.00% <0.00%> (ø)
...of_accounts_importer/chart_of_accounts_importer.py 0.00% <0.00%> (ø)
...ange_rate_revaluation/exchange_rate_revaluation.py 48.97% <0.00%> (ø)
...rpnext/accounts/doctype/fiscal_year/fiscal_year.py 57.14% <0.00%> (ø)
...arges_template/sales_taxes_and_charges_template.py 79.41% <0.00%> (ø)
erpnext/accounts/report/cash_flow/cash_flow.py 42.85% <0.00%> (ø)
...cial_statement/consolidated_financial_statement.py 88.58% <ø> (ø)
...customer_ledger_summary/customer_ledger_summary.py 0.00% <0.00%> (ø)
... and 24 more

@DaizyModi DaizyModi marked this pull request as ready for review November 18, 2022 06:38
@deepeshgarg007 deepeshgarg007 merged commit 7a7b8c2 into frappe:develop Nov 23, 2022
@deepeshgarg007
Copy link
Member

@Mergifyio backport version-14-hotfix

@mergify
Copy link
Contributor

mergify bot commented Nov 23, 2022

backport version-14-hotfix

❌ No backport have been created

  • Backport to branch version-14-hotfix failed
    Pull request with merge commit are not supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accounts needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants