FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add new DeviceReader constructors by bruno-f-cruz · Pull Request #26 · harp-tech/harp-python · GitHub

Add new DeviceReader constructors - #26

Open
bruno-f-cruz wants to merge 47 commits into
mainfrom
feat-device-reader-constructors
Open

Add new DeviceReader constructors#26
bruno-f-cruz wants to merge 47 commits into
mainfrom
feat-device-reader-constructors

Conversation

Copy link
Copy Markdown
Member

Closes #25

bruno-f-cruz requested a review from glopesdev May 9, 2024 15:56

glopesdev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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?

Comment thread harp/model.py Outdated
Comment thread harp/reader.py
Comment thread harp/reader.py
Comment thread harp/schema.py
Comment thread harp/reader.py Outdated
@staticmethod
def from_url(
url: str,
base_path: Optional[PathLike] = None,

glopesdev May 20, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread harp/reader.py
return DeviceReader(device, reg_readers)

@staticmethod
def from_str(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Still looking for a better name:

  • from_yaml: may be misleading since the others also ultimately use YAML
  • from_schema: maybe a little bit better

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 May 20, 2024 16:54
bruno-f-cruz added the feature New planned feature label May 20, 2024
bruno-f-cruz force-pushed the feat-device-reader-constructors branch from ea118f4 to 24b8977 Compare May 20, 2024 19:28

Copy link
Copy Markdown
Member Author

@glopesdev lets review #28 and #27 first and then we will handle this one.

bruno-f-cruz force-pushed the feat-device-reader-constructors branch 2 times, most recently from 57414b4 to 132cb73 Compare March 22, 2025 16:07

Copy link
Copy Markdown
Member Author

@glopesdev I am taking a second look at this and I think we need to mainly decide on a few points:

  1. Method's names (currently):
    -from_model(takes a parsed instance of the device schema, ie. Model class)
    -from_str(takes the literal yml file)
    -from_url(takes an URL to a device yml)
    -from_file(takes the file path to a local device yml)
    -from_dataset(takes the path to a "on-standard" container, replaces the current entry point)

  2. Are any of the methods redundant? (looking at from_file vs from_str for instance)

  3. The fate of base_path. I still think it serves a purpose in the API of the function, but maybe you have a cleaner way in mind.

CLAassistant commented Jun 9, 2025
edited
Loading

Copy link
Copy Markdown


All committers have signed the CLA.

bruno-f-cruz requested a review from glopesdev June 18, 2025 09:41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New planned feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor create_reader into static methods of DeviceReader

3 participants


Back | FazBrowse Home | New Git URL