Issue 12 make outPath accept both str and Path #20

Renfu-Li · 2023-12-20T05:05:36Z

Fixes #12

Changed outPath to snake-case _out_path and its type to str | Path, then added a @property decorator to protect it. This can provide safer access to _out_path, facilitate testing and in general make the code cleaner.

I also modified the README.md in /tests to reflect the directory structure change, and fully resolved outPath for cleaner error messages.

All jobs passed after I manually triggered the "Formatting, Linting and Testing" workflow in my forked repo.

kan-fu · 2023-12-20T22:45:06Z

tests/README.md

 ```

-*To run a single RF test suite (replace 0X with the prefix of the test file name, e.g., 01):*
+_To run a single RF test suite (replace 0X with the prefix of the test file name, e.g., 01):_
+
 ```shell
 robot tests/suites/01*    # robot tests/suites/0X*


Could you also update the path here, line 75 and line87?

Sure, happy to update them

kan-fu · 2023-12-20T22:53:22Z

tests/robot/libraries/delivery.py

@@ -15,7 +17,7 @@ def manualRunProduct(dpRequestId: int):

 def manualDownloadProduct(dpRunId: int, outPath: str = "", resultsOnly: bool = False):
    # Manually downloads runId
-    onc.outPath = outPath


We should discourage users to set instance attribute directly. We could add a setter method in onc.py, but we need to be careful about the naming, because we don't want to change names of public methods at will once released. Mixing camel case with snake case (set_outPath) seems a bad idea to me.

I do plan to migrate to snake case based on PEP 8, I think for now you can use self._out_path, and add a set_out_path(self, out_path: str | Path) method in onc.py. I will bring this up in the internal meeting to see if migration to snake case is too much trouble for end users.

This is going to be a breaking change (But not worth a major version bump I guess).

Does this mean that I can make the changes and commit again in this PR, or should I wait for the decision of internal meeting on migrating to snake case?

You can make the change for now. I can always revert the change if the meeting result is not in favor of snake case.

Thanks for the explanation, I will make the changes and commit again to this branch.

It's common in Python (as opposed to Java) to allow users to set instance attributes directly, especially for something like an ONC.outPath. The name clearly tells what the effects of modifying it does. Moreover, the ONC object doesn't mutate on its own and basically just stores __init__ arguments as common arguments for repeated API calls. But if you need to protect an attribute, there's several ways:

begin with a leading underscore, eg. ONC._out_path to show that it's not part of the public API

Name mangling w/two underscores: ONC.__out_path which, when instantiated as my_onc=ONC(), becomes my_onc.__ONC_out_path. This is primarily used by a superclass to control overloading - not this use case.

@property decorators, which can either generate a set_x method automatically or prohibit them. This is more what you're looking for.

If you want this function to modify outPath but only during this function's execution, you could instead use a context manager to store the old value and temporarily set it as a new value.

Regardless, the longer term solution might simply be to make ONC a TypedDict or dataclass, which would get rid of all the wrappers like

def getDeviceCategories(self, filters: dict = None): return self.discovery.getDeviceCategories(filters)

Thanks for mentioning these options! I think @property is exactly what I want. It even won't break the existing code if we name the setter outPath when internally we still use self._out_path.

I think it is a legitimate requirement for users to change the _out_path, no matter by a setter method, or a direct assignment, and users should not bother the internal implementation. What I concern is not using encapsulation from a developer's perspective, as this might hinder code refactor. Like in this case we are improving code quality but breaking existing code if users manually set self._out_path to string type instead of Path. @property seems a really pythonic way to solve it.

Thanks for your advices, I am going to modify the code to use @property instead.

kan-fu · 2023-12-20T23:35:26Z

src/onc/onc.py

@@ -53,13 +45,18 @@ def __init__(
    def print(self, obj, filename: str = ""):
        """
        Helper for printing a JSON dictionary to the console or to a file
-        @filename: if present, creates the file and writes the output in it
+        @filename: if present, creates a file with a ".json" extension


Nice to catch the bug. It makes sense for the filename to be relative to the outPath.

Do... we really need this function? It isn't used internally, and a generic helper function for dumping a dictionary to a file doesn't seem specific to ONC. Removing it might just help make the code clearer and decrease maintenance.

I totally agree with you. If it is a brand new project, I would not hesitate to remove it, or at least put it in onc.util.py, which contains a bunch of methods not used internally. But unlike the legacy broken files I deleted before, these methods are all working functions and might already be used by end users (especially onc.print, since it is mentioned in the Getting Started doc).

For now I am trying to not break any existing code and keep backwards compatibility.

kan-fu · 2023-12-20T23:43:18Z

I have merged a quick workaround #22 for the problem that the PR from fork cannot use Actions secrets.

Jacob-Stevens-Haas

LGTM, with two suggestions! There's also some theorycraft in whether to allow attribute access (common in python, guarded in Java). Happy with whatever you guys choose, but I'm going to be out over the holidays, so don't let me hold you up.

Jacob-Stevens-Haas · 2023-12-21T16:36:07Z

src/onc/onc.py

-            if outPath[-1] == "/":
-                outPath = outPath[:-1]
-            self.outPath = outPath
+        self._out_path = Path(_out_path)


Would it make sense to fully resolve it as Path(_out_path).resolve() or Path(_out_path).absolute()? That way all error messages utilizing _out_path will be clearer, and any user changing the working directory after instantiating an ONC() won't silently change their _out_path to append the new working directory.

It is a good idea to fully resolve it when printing error messages. But other that, I think self._out_path should work fine without resolving itself.

I meant more for error messages that we don't anticipate, rather than just the ones we raise.

But I guess the real question is, once instantiated/_out_path is set, should an ONC object always point to the same location? In other words, do we expect users who want different downloads in different locations to change their working directory between downloads (we should not resolve), or do we want them to instantiate a new ONC object/use the setter function (we should resolve)?

That is a good question. Different users might adopt different styles, and it should all be fine as long as the code is working properly.

But to me, the design of onc class seems to encourage a singleton pattern because I don't see any benefit of creating multiple objects. I think it is common to change the working directory between downloads to make the download folder more organized (for example grouped by some parameters like locationCode). So I would expect the user to only have one onc object, and change the _out_path when needed, as illustrated in the code example. After this PR is merged, the previous code example still works, and the _out_path will be changed in an encapsulated (setter) way due to @property decorator.

So it seems it should fall into the "we should resolve" category. Could you add resolve at line 33, 52, and revert the change in _util.py? @Renfu-Li

I see. I am going to resolve it as Path(_out_path).resolve() for cleaner error messages

@kan-fu if you're interested, here's my two favorite resources on object-oriented design in python. Not sure if you've seen them, but they might be insightful if you're coming from Java: Stop Writing Classes, a PyCon 2012 talk, and python-patterns.guide, a website by frequent pycon contributor Brandon Rhodes. Here's his blog on the singleton pattern in Python.

Thanks! Indeed all my oop understanding comes from Java. I will take a look.

Jacob-Stevens-Haas · 2023-12-21T16:39:11Z

src/onc/onc.py

@@ -53,13 +45,18 @@ def __init__(
    def print(self, obj, filename: str = ""):
        """
        Helper for printing a JSON dictionary to the console or to a file
-        @filename: if present, creates the file and writes the output in it
+        @filename: if present, creates a file with a ".json" extension


Do... we really need this function? It isn't used internally, and a generic helper function for dumping a dictionary to a file doesn't seem specific to ONC. Removing it might just help make the code clearer and decrease maintenance.

kan-fu · 2023-12-22T03:14:44Z

src/onc/modules/_util.py

-        raise FileExistsError(str(fullPath))
-    with open(fullPath, "wb+") as file:
+    if Path.exists(filePath) and not overwrite:
+        raise FileExistsError(str(filePath))


Could you change this to raise FileExistsError(filePath.resolve()) as Jacob suggested? The str cast seems unnecessary

Sure, I will modify it as suggested

kan-fu · 2023-12-22T04:02:06Z

LGTM, with two suggestions! There's also some theorycraft in whether to allow attribute access (common in python, guarded in Java). Happy with whatever you guys choose, but I'm going to be out over the holidays, so don't let me hold you up.

We will also be off next week so there won't be any PR fom us. Happy holidays!

Renfu-Li · 2023-12-22T18:10:14Z

LGTM, with two suggestions! There's also some theorycraft in whether to allow attribute access (common in python, guarded in Java). Happy with whatever you guys choose, but I'm going to be out over the holidays, so don't let me hold you up.

Thanks for your helpful suggestions. Have a great holiday!

Renfu-Li force-pushed the fix/outPath branch from c647902 to 07e5885 Compare December 20, 2023 22:12

kan-fu requested changes Dec 20, 2023

View reviewed changes

kan-fu requested review from Jacob-Stevens-Haas and eliferguson December 20, 2023 23:43

fix: make outPath accept both str and Path

a3f9a8b

Renfu-Li force-pushed the fix/outPath branch from 5483b57 to c5c6685 Compare December 21, 2023 07:53

Jacob-Stevens-Haas reviewed Dec 21, 2023

View reviewed changes

kan-fu reviewed Dec 22, 2023

View reviewed changes

feat: add @Property decorator to protect _out_path

3d06c76

Renfu-Li force-pushed the fix/outPath branch from c5c6685 to 3d06c76 Compare December 22, 2023 22:11

feat: fully resolve outPath for cleaner error messages

58c6d5d

kan-fu approved these changes Jan 3, 2024

View reviewed changes

eliferguson approved these changes Jan 5, 2024

View reviewed changes

eliferguson merged commit aee93f4 into OceanNetworksCanada:main Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 12 make outPath accept both str and Path #20

Issue 12 make outPath accept both str and Path #20

Renfu-Li commented Dec 20, 2023 •

edited

Loading

kan-fu Dec 20, 2023

Renfu-Li Dec 20, 2023

kan-fu Dec 20, 2023

Renfu-Li Dec 21, 2023

kan-fu Dec 21, 2023

Renfu-Li Dec 21, 2023

Jacob-Stevens-Haas Dec 21, 2023 •

edited

Loading

kan-fu Dec 22, 2023 •

edited

Loading

Renfu-Li Dec 22, 2023

kan-fu Dec 20, 2023

Jacob-Stevens-Haas Dec 21, 2023

kan-fu Dec 22, 2023 •

edited

Loading

kan-fu commented Dec 20, 2023

Jacob-Stevens-Haas left a comment

Jacob-Stevens-Haas Dec 21, 2023

kan-fu Dec 22, 2023

Jacob-Stevens-Haas Jan 2, 2024

kan-fu Jan 2, 2024

Renfu-Li Jan 2, 2024

Jacob-Stevens-Haas Jan 3, 2024

kan-fu Jan 3, 2024

Jacob-Stevens-Haas Dec 21, 2023

kan-fu Dec 22, 2023

Renfu-Li Dec 22, 2023

kan-fu commented Dec 22, 2023

Renfu-Li commented Dec 22, 2023

Issue 12 make outPath accept both str and Path #20

Issue 12 make outPath accept both str and Path #20

Conversation

Renfu-Li commented Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacob-Stevens-Haas Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

kan-fu Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kan-fu Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

kan-fu commented Dec 20, 2023

Jacob-Stevens-Haas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kan-fu commented Dec 22, 2023

Renfu-Li commented Dec 22, 2023

Renfu-Li commented Dec 20, 2023 •

edited

Loading

Jacob-Stevens-Haas Dec 21, 2023 •

edited

Loading

kan-fu Dec 22, 2023 •

edited

Loading

kan-fu Dec 22, 2023 •

edited

Loading