PR template introduction#16
PR template introduction#16AhmetCanSolak merged 2 commits intonapari:masterfrom AhmetCanSolak:prtemplate
Conversation
|
Some general comments: in my opinion, less is more with PR templates. We have a much shorter list in scikit-image and it has had quite a limited effect on PR quality or even checklist compliance, as far as I can tell. I think the only thing that is absolutely required is complete test coverage of any changes, together with measurements from continuous integration systems (so that is the first thing that I would set up). In contrast, the style guide (which you allude to, but needs to exist before this template is adopted) and contributing guide are very important. People don't usually read them ahead of time, but (a) having CONTRIBUTING.md means they get a big yellow warning banner when submitting a PR, and (b) once people submit a PR, it's very quick to prod them to comply by pointing them to specific clauses in the contributing guide. This kind of personal ping is much more successful than the template. I suggest you look at the scikit-image contributing guide and template, and maybe even copy the former wholesale. As I mentioned, I think the template should be minimized: even the one we have in scikit-image is too much in my opinion. |
| - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) | ||
| - [ ] This change requires a documentation update | ||
|
|
||
| ## How Has This Been Tested? |
There was a problem hiding this comment.
I don't think that this section is necessary. Reviewers need only look through the files to find the proper test cases (which they should be doing in the first place).
There was a problem hiding this comment.
I agree with you on short-run but might be useful when there are many people contributing.
…visible
The visibilitychanged slot currently updates the title regardless of the visibility. We are seeing a crash when the viewer is shut down and a signal is sent to the visibilitychanged slot. That executes some code which causes a segfault in the python process[1]. This PR changes the visibility changed slot to only update the title bar if the widget is visible.
[1]
```
* frame #0: 0x0000000115cc96e4 QtWidgets`QWidgetPrivate::reparentFocusWidgets(QWidget*) + 356
frame napari#1: 0x0000000115cb99cf QtWidgets`QWidget::setParent(QWidget*, QFlags<Qt::WindowType>) + 911
frame napari#2: 0x0000000115cb82c0 QtWidgets`QWidgetPrivate::init(QWidget*, QFlags<Qt::WindowType>) + 688
frame napari#3: 0x0000000115d73e5e QtWidgets`QFrame::QFrame(QFramePrivate&, QWidget*, QFlags<Qt::WindowType>) + 14
frame napari#4: 0x0000000115dc3705 QtWidgets`QLabel::QLabel(QWidget*, QFlags<Qt::WindowType>) + 277
frame napari#5: 0x0000000116503189 QtWidgets.abi3.so`Sbk_QLabel_Init(_object*, _object*, _object*) + 1497
frame napari#6: 0x000000010015f84d Python`wrap_init + 12
frame napari#7: 0x0000000100113e64 Python`_PyObject_FastCallDict + 143
frame napari#8: 0x00000001001b17a2 Python`call_function + 439
frame napari#9: 0x00000001001aa46b Python`_PyEval_EvalFrameDefault + 3078
frame napari#10: 0x00000001001b1ee2 Python`_PyEval_EvalCodeWithName + 1638
frame napari#11: 0x00000001001b281b Python`_PyFunction_FastCallDict + 447
frame napari#12: 0x0000000100113e99 Python`_PyObject_FastCallDict + 196
frame napari#13: 0x0000000100113fa3 Python`_PyObject_Call_Prepend + 131
frame napari#14: 0x0000000100113d1e Python`PyObject_Call + 101
frame napari#15: 0x000000010015f7d1 Python`slot_tp_init + 57
frame napari#16: 0x000000010015c782 Python`type_call + 178
frame napari#17: 0x0000000100113e64 Python`_PyObject_FastCallDict + 143
frame napari#18: 0x00000001001141fa Python`_PyObject_FastCallKeywords + 97
frame napari#19: 0x00000001001b17a2 Python`call_function + 439
frame napari#20: 0x00000001001aa4fb Python`_PyEval_EvalFrameDefault + 3222
frame napari#21: 0x00000001001b28e0 Python`_PyFunction_FastCall + 110
frame napari#22: 0x0000000100113e99 Python`_PyObject_FastCallDict + 196
frame napari#23: 0x0000000100113fa3 Python`_PyObject_Call_Prepend + 131
frame napari#24: 0x0000000100113d1e Python`PyObject_Call + 101
frame napari#25: 0x000000011126f146 libpyside2.abi3.5.14.dylib`PySide::SignalManager::callPythonMetaMethod(QMetaMethod const&, void**, _object*, bool) + 534
frame napari#26: 0x000000011126eb77 libpyside2.abi3.5.14.dylib`PySide::SignalManager::qt_metacall(QObject*, QMetaObject::Call, int, void**) + 519
```
With this PR, I am introducing a PR template to keep a standard on our development process and make things easier for possible future contributor friends.