-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new DeviceReader constructors #26
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.toml
in 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?
harp/model.py
Outdated
from pydantic import (BaseModel, BeforeValidator, ConfigDict, Field, RootModel, | ||
field_serializer) | ||
from typing_extensions import Annotated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
harp/reader.py
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, all other changes are from running isort.
harp/schema.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
a5c22e4
to
ea118f4
Compare
ea118f4
to
24b8977
Compare
@glopesdev lets review #28 and #27 first and then we will handle this one. |
* Add `isort` and `codespell` linters and respective settings * Increase line length to 108 * Remove isort line length override
Remove redundant file mode
Avoid silent overflow for numpy compatibility
Add CI pipeline and code coverage
…harp-tech/harp-python into feat-device-reader-constructors
Closes #25