Add new DeviceReader constructors #26
Conversation
There was a problem hiding this comment.
@bruno-f-cruz thanks, looks good overall. I have a few questions / comments:
- Would be nice if bf816e2 would be in its own PR so that it gets listed in the release notes. Is it too hard to extract? Doesn't look like the rest of the PR is depending on it.
- What linter are you using? Can you run black on the current repo state? When I run it on my end I'm getting a different result. Maybe we could agree on what tools we use and then add them to
pyproject.tomlin a separate PR. - I'm having second thoughts about the use of
base_path. I don't like that it is used in different ways depending on which function gets called, so maybe it would be best to get rid of it entirely?
| from pydantic import (BaseModel, BeforeValidator, ConfigDict, Field, RootModel, | ||
| field_serializer) | ||
| from typing_extensions import Annotated |
There was a problem hiding this comment.
| from pydantic import (BaseModel, BeforeValidator, ConfigDict, Field, RootModel, | |
| field_serializer) | |
| from typing_extensions import Annotated | |
| from typing_extensions import Annotated | |
| from pydantic import BaseModel, BeforeValidator, ConfigDict, Field, RootModel, | |
| field_serializer |
I would keep typing_extensions close to typing since they are related both in name and functionality.
Also running black on my end is touching this file, since it thinks the line is too long.
There was a problem hiding this comment.
Fixed it by running black.
Regarding the order I am tempted to leave it as is, as it is the suggested sorting order given by isort (the new linter I included in the project.toml). Let me know if this makes sense. If not we can remove isort as a depedendency and go with what we had
| from collections import UserDict | ||
| from dataclasses import dataclass | ||
| from datetime import datetime | ||
| from functools import partial | ||
| from math import log2 | ||
| from os import PathLike | ||
| from pathlib import Path | ||
| from datetime import datetime | ||
| from functools import partial | ||
| from dataclasses import dataclass | ||
| from typing import (Any, BinaryIO, Callable, Iterable, Mapping, Optional, | ||
| Protocol, Union) | ||
|
|
||
| import requests | ||
| from deprecated import deprecated | ||
| from numpy import dtype | ||
| from pandas import DataFrame, Series | ||
| from typing import Any, BinaryIO, Callable, Iterable, Mapping, Optional, Protocol, Union | ||
| from collections import UserDict | ||
| from pandas._typing import Axes | ||
| from harp.model import BitMask, GroupMask, Model, PayloadMember, Register | ||
|
|
||
| from harp import __version__ | ||
| from harp.io import MessageType, read | ||
| from harp.model import BitMask, GroupMask, Model, PayloadMember, Register | ||
| from harp.schema import read_schema |
There was a problem hiding this comment.
Is there any real change here besides importing __version__? In general as long as black is happy with it I would prefer minimizing diffs on imports so it can be easy to understand what changed in terms of dependencies from version to version.
There was a problem hiding this comment.
You are correct, all other changes are from running isort.
harp/schema.py
Outdated
There was a problem hiding this comment.
Same as above, is there a reason to reorder the imports in this file? If this comes from some automatic linter, maybe we can agree on what tools to use so we can avoid reformatting each other's files?
| @staticmethod | ||
| def from_url( | ||
| url: str, | ||
| base_path: Optional[PathLike] = None, |
There was a problem hiding this comment.
base_path is indeed getting clunky, I'm slightly uncomfortable of having different behaviors depending on which function gets called here, so maybe just getting rid of it would be best.
There was a problem hiding this comment.
Not sure I follow. The use of base_path is consistent across methods. If provided it will try use it as a root directory where all files are expected to be found. If not provided it will assume either the current working directory (e.g. from_url) or the root of the provided yml (e.g. from_file). Are these two defaults what are raising flags for you? Happy to go with a different one if you want. We can also just throw an error at the level of the DeviceReader class if no path was provided and the user try to use the no-input overload of the register load function.
| return DeviceReader(device, reg_readers) | ||
|
|
||
| @staticmethod | ||
| def from_str( |
There was a problem hiding this comment.
Still looking for a better name:
from_yaml: may be misleading since the others also ultimately use YAMLfrom_schema: maybe a little bit better
There was a problem hiding this comment.
from_schema makes me nervous as people might think you should pass the actuall schema file (i.e. .yml). Would from_string be better (ToString is also a thing after all haha)?
bruno-f-cruz
force-pushed
the
feat-device-reader-constructors
branch
from
a5c22e4
to
ea118f4
Compare
bruno-f-cruz
force-pushed
the
feat-device-reader-constructors
branch
from
ea118f4
to
24b8977
Compare
|
@glopesdev lets review #28 and #27 first and then we will handle this one. |
Closes #25