doc: test: suggest multi-line imports in functional test style guide#25811
Conversation
|
ACK 4edc689 |
|
Thanks for pointing this out, I hadn't considered merge conflicts in my comment but I'm honoured to be an inspiration. Reducing maintenance burden seems a good reason for these updated style guidelines. We would still have merge conflicts for initially single name imports that then become multiple names, as showcased in the below script. Would it then be better to apply a uniform rule and always have imports be multiline? Either way, I think the rest of from collections import defaultdict
...
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
)Example merge conflict for importing single -> multiple namesmkdir mergeconflict && cd mergeconflict && git init && git checkout -b master
echo “from typing import List” > main.py && git add main.py && git commit -m “init”
git checkout -b a
echo "from typing import (\n List,\n Any,\n)" > main.py && git commit main.py -m "import any"
git checkout master && git checkout -b b
echo "from typing import (\n Tuple,\n List,\n)" > main.py && git commit main.py -m "import tuple"
git checkout master && git merge a
git checkout b && git rebase master |
|
Concept ACK. AFAICT this is preferred and for this reason, so might as well document it. |
…nal test style guide 4edc689 doc: test: suggest multi-line imports in functional test style guide (Sebastian Falbesoner) Pull request description: As long as I remember contributing to functional tests (~2-3 years), it was always kind of an unwritten rule that multi-line imports are preferred over single-line imports in order to reduce the possibility of potential merge conflicts -- at least if more than one symbol from a module is imported. This PR adds this rule to the style guide and adapts the example test accordingly. (Inspired by bitcoin#25792 (comment)). ACKs for top commit: kouloumos: ACK 4edc689 1440000bytes: ACK bitcoin@4edc689 w0xlt: ACK bitcoin@4edc689 fanquake: ACK 4edc689 Tree-SHA512: c7b6ff62f601f4e57cc8334d291662987d6737ebca8d81c063280409f4412302172f1404ec16afc9a13007bcdba55bdab66b9b80363287e287888929cb386584
Thanks for testing out the merge-conflict-scenario for single-/multi-line imports! I'd personally be fine with applying an uniform rule as it's more clear. Though one could counter-argue that there are some modules from where in almost all cases we only import one symbol, and it's probably not worth it to add two extra lines, e.g. the following, being present in every single functional test: Anyways, if you (or anyone else) plans a follow-up, feel free to tag me as reviewer. |
8 participants
As long as I remember contributing to functional tests (~2-3 years), it was always kind of an unwritten rule that multi-line imports are preferred over single-line imports in order to reduce the possibility of potential merge conflicts -- at least if more than one symbol from a module is imported. This PR adds this rule to the style guide and adapts the example test accordingly. (Inspired by #25792 (comment)).