It’s a brand new year. And a brand new UI (soon). I think it’s time for a brand new developer experience as well.
As a part of this experience, I think introduction of code formatters will have a huge impact on developer productivity and peace of mind. Don’t take it from me, read what an engineer from Google has to say.
There are a couple of ways to go about code formatting:
We can opt for opinionated (but wildly popular!) code formatters. Pros:
Code is consistent with some of the most popular and widely adopted practices.
Allows developers coming from other repositories to quickly understand Frappe’s code style.
No time spent debating over code formatter configuration.
Cons:
Some of the opinions implemented into these formatters may be disliked by some developers and might need time getting used to (e.g. spaces over tabs).
Alternatively, we can opt for configurable code formatters which may be easier to switch to, but at the cost of having a code style that:
Will differ from widely adopted practices.
New developers will take time to get used to.
Will get debated upon.
What are your thoughts? Please vote here:
Opinionated Code Formatters - Black (Python) and Prettier (JS)
Configurable Code Formatters - YAPF (Python) and JS Beautify
Nah, code formatters are overrated
0voters
If we implement this successfully, it can be a great first step towards having a detailed style guide that is easy to understand and adopt (just like ERPNext is!) and leads to cleaner and more consistent code throughout frappe, erpnext and bench.
I just wanted to clear something out.
When we are contributing to frappe or erpnext which code formatter/extension should we use so that it doesn’t disturb the prevailing code style?
I formatted frappe and erpnext code with black (no config specified) and got following result.
For frappe repo
error: cannot format /home/revant/frappe-bench/apps/frappe/frappe/core/doctype/data_import/test_exporter.py: INTERNAL ERROR: Black produced invalid code: unexpected indent (<unknown>, line 92). Please report a bug on https://github.com/psf/black/issues. This invalid output might be helpful: /tmp/blk_grt0bfui.log
error: cannot format /home/revant/frappe-bench/apps/frappe/frappe/event_streaming/doctype/event_update_log/event_update_log.py: INTERNAL ERROR: Black produced code that is not equivalent to the source. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_pmw9d16w.log
error: cannot format /home/revant/frappe-bench/apps/frappe/frappe/desk/reportview.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_x6nz44al.log
error: cannot format /home/revant/frappe-bench/apps/frappe/frappe/core/doctype/data_import/importer.py: INTERNAL ERROR: Black produced invalid code: unindent does not match any outer indentation level (<unknown>, line 923). Please report a bug on https://github.com/psf/black/issues. This invalid output might be helpful: /tmp/blk_itz3vsuz.log
error: cannot format /home/revant/frappe-bench/apps/frappe/frappe/core/doctype/communication/communication.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_7_618v7t.log
error: cannot format /home/revant/frappe-bench/apps/frappe/frappe/model/base_document.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_ghppzpo9.log
error: cannot format /home/revant/frappe-bench/apps/frappe/frappe/__init__.py: INTERNAL ERROR: Black produced code that is not equivalent to the source. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_0vf5se13.log
Oh no! 💥 💔 💥
931 files reformatted, 373 files left unchanged, 7 files failed to reformat.
error: cannot format /home/revant/frappe-bench/apps/erpnext/erpnext/regional/germany/utils/datev/datev_csv.py: INTERNAL ERROR: Black produced code that is not equivalent to the source. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_zwh9v_f0.log
error: cannot format /home/revant/frappe-bench/apps/erpnext/erpnext/accounts/doctype/exchange_rate_revaluation/exchange_rate_revaluation.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_c3__761t.log
error: cannot format /home/revant/frappe-bench/apps/erpnext/erpnext/manufacturing/report/exponential_smoothing_forecasting/exponential_smoothing_forecasting.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_0hr4s9f8.log
error: cannot format /home/revant/frappe-bench/apps/erpnext/erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_y56wxsch.log
error: cannot format /home/revant/frappe-bench/apps/erpnext/erpnext/stock/doctype/stock_entry/test_stock_entry.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_he98dv4j.log
error: cannot format /home/revant/frappe-bench/apps/erpnext/erpnext/stock/get_item_details.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_10jc4jxa.log
error: cannot format /home/revant/frappe-bench/apps/erpnext/erpnext/accounts/doctype/journal_entry/journal_entry.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_slovmwlp.log
error: cannot format /home/revant/frappe-bench/apps/erpnext/erpnext/controllers/accounts_controller.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk___61m9xd.log
Oh no! 💥 💔 💥
2419 files reformatted, 1241 files left unchanged, 8 files failed to reformat.
I recommend installing black globally on CI runner or dev machine instead of adding it to requirements.txt as it will add few unnecessary MB to production environment. Doesn’t matter much by addition of few MB in already 500MB+ frappe py environment. Just my thought.
Run black --check as part of CI.
Write about formatting in docs or repo README(s) for developers to format their code before sending PR.
From a technical perspective? No problem. A short script to loop through every Python file. Use built-in tools like sed and awk to find and replace the tabs. If a line begins with 1+ tabs, replace it. It’s been a few years, but I think I tried doing this with Bench. Worked fine. Ran a linter to identify issues afterward.
From a community perspective? I suspect that’s the headache Frappe wants to avoid. Otherwise they’d have switched to Tabs a decade ago. The community Apps, modules, forks, tools, and custom integrations would need to follow along, and change their Tabs to spaces too.
Would love to see it happen. Tabs do occasionally cause a small headache for me.
But honestly, it’s not a big deal anymore. I am used to them. I have configurations for all my editors (though I had to fork one of them). Flake8 and Pylint do a good job recognizing mistakes, and making things pretty. It’s not a big deal.
Overall regarding this discussion. I defer to the main Contributors. They’re spending the most-time with the code. Would love to know their opinions on this topic.
General comment: Project maintenance can’t happen by committee. If there is voting, it has to be among the top 10 contributors. If the top 5 / 10 contributors have a buy-in, then yes, else no. Or become a top 5 contributor yourself.
^ Pretty much this. I might broaden to Top 20, to involve people like @rmeyer who have been doing a lot. But I agree with you, @rmehta. Those contributing to the code should call the shots for these types of discussions.
Technically, I don’t think this is necessary. The code works just like it would, even if one of the files in an app that uses Tabs, uses spaces. Likewise, I don’t think it will be a problem if community apps still use tabs.
Using code formatters in a world of opinionated code formatters is a no-brainer. However, my only problem with this is the git history problem. Even if git has support for ignoring certain revisions, the GitHub UI doesn’t support it, and almost everyone uses the GitHub UI for viewing history and blames. Is there a way to not mess up git history that works on GitHub as well?