Closed paddyroddy closed 1 year ago
Preview page for your plugin is ready here: https://preview.napari-hub.org/quantumjot/BayesianTracker/204 Updated: 2023-03-07T15:43:42.943562
Patch coverage: 87.98
% and project coverage change: +1.01
:tada:
Comparison is base (
0c61b83
) 87.53% compared to head (43ff60d
) 88.54%.
:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Good news: everything is passing now. Bad news: this is a huge pr with a lot of changes. I'll try to summarise them here:
tox and github actions changed:
tox.ini
for napari tests to run in CItype hint changes:
Union
with the |
operatorList
, Tuple
, etc.) with standard collections (list
, tuple
etc.)numpy.typing
to replace np.ndarray
with npt.NDArray
pydantic typing overrides:
Union
and List
instead of |
and list
annotations
from __future__
, pydantic will still complain that e.g. list
is not subscriptablelots of formatting changes made by black
, e.g. replace single quotes with double quotes
remove magic numbers and replace with constants, e.g. TWO_DIM = 2
modifying tests is usually a bad idea, but there are a couple of changes made here
create_test_object
, rename the parameter id
to test_id
so that we do not use the name of a builtin some changes we might want to reverse:
models/particle_config.json
Hypothesis.type
has been renamed to Hypothesis.hypothesis_type
, again to avoid using the name of a builtin. But this would be a breaking change for users and so probably a bad ideaLet me know if there's anything else that is not clear!
In general, I think this look great. I don't have time to thoroughly go through this, but a couple of thoughts:
modifications to format of the data files, e.g. models/particle_config.json
generally is fine, except we lose some readability from the users perspective - in particular, the matrices are not shaped correctly due to the formatting changes.
Hypothesis.type
has been renamed toHypothesis.hypothesis_type
, again to avoid using the name of a builtin. But this would be a breaking change for users and so probably a bad idea
This is fine (actually good) - this is not really a public interface anyway, so users shouldn't be directly setting these.
in particular, the matrices are not shaped correctly due to the formatting changes
okay, I'll make the matrices look nice again but leave the other formatting changes
This is fine (actually good) - this is not really a public interface anyway, so users shouldn't be directly setting these.
thanks for the clarification - let's stick with the new name then
I've made those few changes and I think this is ready to be merged now, but would you prefer me to wait until #205 is merged first @quantumjot ?
I would go ahead and merge. #205 is pretty simple, but might need a new test adding
This PR adds
ruff
with a lot of rules and changes (sorry). Anything unclear/disputed, please ask away. Have also changed the line length from79
to88
as I couldn't see a reason not to.I haven't tried to add
mypy
here (#164). One for a separate PR.