added poethepoet package to manage repetitive development tasks #304

qlrd · 2024-01-04T16:45:17Z

Description

Poe the poet

Poethepoet is a nice task runner that works well with poetry.

We can define a [tool.poe.tasks] section on pyproject.toml and many tasks with the following
format:

[tool.poe.tasks]
_task1 = "<some poetry command>"
_task2 = "<some shell command>"
task_A = ["_task1", "_task2"]
_task3 = "<some poetry command>"
_task4 = "<some shell command>"
task_B = ["_task3", "_task4"]

Where the prepended _ means that specific task cannot be called directly and commands can be nested in a array; and a task without _ can be called by:

poetry run poe task_A
poetry run poe task_B

Maybe its save some time for developers, mainly in formatting, lint, testing and simulation commands.

If approved, it's worth to update README with these "tasks"

Simulator

To enable the poe task manager in the simulator, it was necessary to change the way in which it looked for the path of some modules, from a relative way, to an absolute way.

Simulator tasks

Was added many simulator sequences that can be activated by

poetry run poe simulator-prepare
poetry run poe simulator-sequence-<device>-<op>
poetry run poe simulator-sequence-<device> # runs all <ops>

Where <device> can be one of the supported and <op> the operation to be sequenced

Lint tests

When wrapping lint into multiple folders, we include some lints in the testing directory

Docstrings

Added many docstrings on test files.

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other: improvements on development process

added lint to firmware/scripts/rgbconv.py

Improved some docstrings in src/krux/context.py

…w.py

…iew.py

…adability of long lines

…at lead to additional limitrof characters

added black formating to test/test_encryption.py

added a docstring to tests/test_format.py

odudex · 2024-01-07T22:11:06Z

I didn't to the time to review this PR, but while crossing my eyes over it I got afraid about the amount amount of text and comments. Will we adopt things like "param", "return" on every docstring? Redundant information confuses me, makes it harder to find things, scrolling or using "ctrl+F, H". If we go this way would rather lock the params and return types at the function declaration themselves, without comments about them. My background in programing and good practices is poor, so I would like to hear from @tadeubas and @jdlcdl too.
Also, for changes in main code I would like more specific PRs, with defined purpose: What is the feature, issue or demand is it solving.
I promise I'll review all more quickly.

qlrd · 2024-01-08T09:56:16Z

@odudex, thank you for your attention. Here are some justifications:

I got afraid about the amount amount of text and comments.

Some of the comments are pylint macros and others are explanations about why i put some these pylint macros (just to give a reason why I did that). But I certainly didn't plan to keep them, just to let you know for possible approval or not.

Will we adopt things like "param", "return" on every docstring? For me, redundant information confuses me, makes it harder to find things, scrolling or using "ctrl+F, H". If we go this way would rather lock the params and return types at the function declaration themselves, without comments about them.

I thinked to that lock the params and return types is better than add them to docstrings, but was afraid break the things if micropython support or not support it. If you agree, I am happy to remove the references in the docstrings and try to add them to the function declarations.

Also, for changes in main code I would like more specific PRs, with defined purpose: What is the feature, issue or demand is it solving. I promise I'll review all more quickly.

I will avoid 🤞🏾

The most of the changes were to docstrings (I can remove them as proposed above).

Any changes to their functionality was avoided. Changes to the main code only occurred when pylint warned, such as calling a variable with the same name as those pre-defined by python itself.

qlrd · 2024-01-08T15:54:36Z

@odudex, checked some of the merge conflicts and fixed them according what i saw in the selfcustody/integrated_changes.

Removed some of the :params things on docstrings

odudex · 2024-01-08T19:03:05Z

I started fixing tests now, so probably some more conflicts may show up.
Let's wait others to answer what they have to say about about :params. After we define a position about it, we standardize. If we decide to do it, we can gradually implement it, if not, we should remove existing ones.

tadeubas · 2024-01-08T21:03:07Z

Regarding :param: in the code docs, I think it's too detailed to explain what the variable is and the type... I think if we need to comment on a variable, it's a sign of a problem... the name of the variable must be changed to resolve the need for the comment. The same applies to return comments. Python is great because of its relatively simple syntax and versatility. If we pollute the code (or code documentation) with too many things, it will make it look like good old languages like Java, not like Python!

Again I think if we need to comment on this, it's a sign that we need to refactor this code, change its name, its parameters, the parameter names, change the way the function/class works, break it into small pieces and/or put it in different files.

tadeubas · 2024-01-08T21:12:53Z

firmware/scripts/minify.py

+
+# TODO: what this script do?
+"""
+Some documentation about this script


It is self explanatory I think, it minifies the files passed as arguments. The output is the file minified (without extra new lines or code comments)

I think i wanted more details how this is done (and projected it), sorry

tadeubas · 2024-01-08T21:16:18Z

firmware/scripts/rgbconv.py

    """
+
+    # TODO: this throw a pylint warn C0123: Use isinstance() rather than type() for a typecheck.


Hi @qlrd These are just helpers (scripts) to serve as a tool to do something not direct related to the Krux code (like creating values to put those on code, or to help with the build process). These scripts will not ship into the Krux device... I think if you really want to change this, you could use isinstance() without any problems. We already use it a lot on the code, see for example the file wallet.py

Put this because pylint was warning

tadeubas · 2024-01-08T21:21:38Z

firmware/scripts/rgbconv.py

+        """
+        return (2**number_of_bits) - 1
+
+    # TODO: this throw a pylint warn C0123: Use isinstance() rather than type() for a typecheck.


plz see the comment above

tadeubas · 2024-01-08T21:26:03Z

firmware/scripts/rgbconv.py



-if __name__ == '__main__':


For those type of changes (from single quotes to double quotes), it is nice to read about it here. Basically I think we already uses single quotes for fixed small strings or constants, and double quotes to the other uses

I think this was black autoformating 😵‍💫

tadeubas · 2024-01-08T21:34:26Z

pyproject.toml

+]
+
+# m5stickv
+simulate-sequence-m5stickv-about = "python simulator/simulator.py --sequence simulator/sequences/about.txt  --device maixpy_m5stickv"


We could use more the variables and less fixed names, so we have more code reuse instead of copy/paste. For example, when you use a simulator sequence you use the start of the name of the file and hit tab to use the terminal autocomplete feature, that way you don't need to figure the exact name of the file. I don't know if the terminal will autocomplete this name simulate-sequence-m5stickv-bitcoin-options but we could remember about simulate-sequence and then only pass the parameters (the sequence file and the device). Or simulate-sequence-m5stickv and only pass the sequence file

I can fix this changing the .sh files, but i was afraid to break the procedures. So i coded in this manner to see what would you say.

tadeubas · 2024-01-08T21:36:04Z

simulator/kruxsim/devices.py

@@ -36,48 +36,76 @@
    DOCK: (440, 820),
 }

-
-def with_prefix(device):
+##################################


Is this docs or a simple comment? if it is docs, we should use triple quotes """

tadeubas · 2024-01-08T21:39:31Z

simulator/kruxsim/devices.py

    device = with_prefix(device)
    if device not in fonts:
+        # Get the current dir of current file
+        # to dynamically get the absolute path of bdf font


too much comments, the dirname already tells it will return the name of the directory of the file passed. abspath already tells it will return the absolute path of the file passed.

tadeubas · 2024-01-08T21:44:25Z

simulator/simulator.py

@@ -78,32 +79,51 @@
 board.register_device(args.device)

 if args.sd:
+    # pylint: disable=unused-import


I think we can customize pylint so we don't have to pollute the code with all these comments

tadeubas · 2024-01-08T21:45:13Z

simulator/simulator.py

@@ -113,14 +133,25 @@
 sensor.register_sequence_executor(sequence_executor)
 ft6x36.register_sequence_executor(sequence_executor)

+##################################
+# Handle absolute path for files #
+##################################


Why this box, it make this looks like docs but it is a comment.

Put this only to clarify that for the simulator to work on calls in the current working directory, the full path was needed

tadeubas · 2024-01-08T21:48:06Z

src/krux/pages/pub_key_view.py


    def __init__(self, ctx):
        super().__init__(ctx, None)
        self.ctx = ctx
        self.utils = Utils(self.ctx)

+    # TODO: Is it worth to define the functions inside as protected members of PubkeyView?
+    # (to aim a better readability)


I think this is a discussion that may happen on a discussion or an issue, not on the code. But what you mean by define the function inside as protected? what functions? _save_xpub_to_sd, _pub_key_text, _pub_key_qr? They are already defined in a protected manner, the function public_key() is used and need to be callable inside the class PubkeyView

I put this because pylint warns. I called protected in the definition used by pylint, those object methods that starts _, inside a class definition, but outside from another method. I was following the pylint standards and wanted all pass in poetry run poe lint command. But we don't need to follow, if you prefer

tadeubas · 2024-01-08T21:54:55Z

src/krux/pages/qr_view.py

    def _region_legend(self, row, column):
+        """
+        <DOCUMENTATION HERE>


Don't add documentation without adding documentation plz 😆 This will make pylint to pass and nobody will see this anymore!

tadeubas · 2024-01-08T22:00:02Z

tests/shared_mocks.py

                return mock.mock_open(read_data=content).return_value
        raise OSError("(mock) Unable to open {filename}")

    return mock.MagicMock(side_effect=open_mock)


 def statvfs(_):
+    """
+    TODO: proper documentation


I don't think it's necessary to cover the tests with all this rigor, as we do with the code. And plz don't add a code docs with TODO add docs 😆

I added TODO because i created a poe task to follow TODOS in all the code. But wither way, agreed

tadeubas · 2024-01-08T22:05:10Z

tests/test_baseconv.py

+            b"\xb7\xb7\xe9\xc7z\xdf}\xdb\xe5\xc6\xdc\xdf\xdd4\xd3M4\xd3M4}\xf7\xdf}\xf7\xdf\xd3V",
+            b"\xb4\xe5\xa7\x9a\xd1\xbd4\xd3M4\xd3M}\xef\xa6\xbd\xd7\x87\xdf{\xd74\xd3\xadt\xf7",
+            b"\xb7\x1c\xdd\xbe\xb7\xe9\xfd\x9coN:\xd1\xf6\xb8}\xce6\xed\xdd\x9b\xe3\x9f<i\xcd4",
+            b"\xd3M4\xd3M4\xd3]5\xdbOy\xe5\xe7\x9a\xd1\xbd4\xd3M4\xd3M{k\xddx\xeb~9\xdbM\x1f\xeb",


These consts are copied and pasted once to use on code as tests, so it is harder to format them like you did. They are just consts for tests, we don't need to format them that way, I think wha you did will not help anyone understand better what this const is and what it's value... we should rely on the name of the variable or a helpful comment to add better understanding of those consts

Formated these because pylint

tadeubas · 2024-01-08T22:06:10Z

tests/test_camera.py

-    SNAP_HISTOGRAM_FAIL,
+    # TODO: fix W0611: Unused SNAP_HISTOGRAM_FAIL imported from shared_mocks
+    # this will be used in a near future or can be removed?
+    # SNAP_HISTOGRAM_FAIL,


if it is unused, just remove. If we need it in the future, we will add it again

I was afraid to break a future (or wanted) implementation...
So i put the comment to just to warn

it is a test file, just execute the tests before your change and after poetry run pytest --cache-clear .\tests\test_camera.py

If everything it the same, it was a safe change... so you can remove this TODO comment

qlrd · 2024-01-08T22:21:33Z

Thanks @tadeubas,

Regarding :param: in the code docs, I think it's too detailed to explain what the variable is and the type... I think if we need to comment on a variable, it's a sign of a problem... the name of the variable must be changed to resolve the need for the comment. The same applies to return comments.

Maybe this can be fixed with typings in function declarations?

Python is great because of its relatively simple syntax and versatility. If we pollute the code (or code documentation) with too many things, it will make it look like good old languages like Java, not like Python!

I put it more as a proposal from someone who is seeing the code for the first time and, when using editors or IDEs, I like to hover over a method and a class and see which inputs are valid or not, what it does, etc. (but it's my demand, it may not be from others...)

tadeubas · 2024-01-08T23:17:19Z

Sure, I think we can specify the type of a parameter and the type of the return of a function, like we see on this discussion. We should just make one test to see if this will be a problem when compiling to micropython and putting on the device

qlrd · 2024-01-09T18:22:25Z

Sure, I think we can specify the type of a parameter and the type of the return of a function, like we see on this discussion. We should just make one test to see if this will be a problem when compiling to micropython and putting on the device

Found this

From what i understand, is possible using primitives like those defined in CPython, e.g.:

def format_btc(amount: int) -> str:

But for more specialized arguments, typing module is desired. There's an opened PR implementing an optimized typing module for micropython like those used in "normal" python. But our micropython is forked one, right?

Another comment: The MicroPython compiler itself supports type annotations, so in principle the typing module will work

qlrd · 2024-01-10T17:11:39Z

Closing PR since we need to break commits in smaller PRs

qlrd added 20 commits January 4, 2024 13:40

added poethepoet package to manage repetitive development tasks

dccfe92

added more poe tasks for simulation on amigo and dock

a148d1d

added TODO listing task for checking TODOS in some files

a4ca983

added lint to firmware/scripts/rgbconv.py

improved list-todos* tasks

01c3209

added some TODOs to firmware scripts

5e53d09

Added some lint and improved docstrings to src/krux/encryption_ui.py

28a6cf0

Improved some docstrings in src/krux/context.py

added some lint and improved docstrings on src/krux/pages/pub_key_vie…

895e62f

…w.py

added lint, formatting and improved docstrings on src/krux/pages/qr_v…

8a02cfa

…iew.py

added some lint to tests/conftest.py

4a2f3f0

added some black formating to tests/conftest.py

f986ab1

fixed some multiline pylint macro

ce4c028

tests/shared_mocks.py formated and linted

5618d85

added some formatting and lint to tests/test_baseconv.py to better re…

94291f2

…adability of long lines

fix line-long lint on tests/test_baseconv.py after black formating th…

a65142e

…at lead to additional limitrof characters

added black formatting and lints to test/test_camera.py

9419440

added some black formatting and lint to test/test_context.py

179c345

added black formatting and lint to tests/test_display.py

16cf12c

added docstrings and some lint to tests/test_encryption.py

6336561

added some linting and docstrings to tests/test_firmware.py

82ad974

added black formating to test/test_encryption.py

added a :param docstring to src/krux/format.py

7f6cc74

added a docstring to tests/test_format.py

Merge branch 'integrated_changes' into integrated_changes

11d173a

tadeubas reviewed Jan 8, 2024

View reviewed changes

qlrd closed this Jan 10, 2024

		"""

		# TODO: this throw a pylint warn C0123: Use isinstance() rather than type() for a typecheck.

added poethepoet package to manage repetitive development tasks #304

added poethepoet package to manage repetitive development tasks #304

Conversation

qlrd commented Jan 4, 2024 • edited Loading

Description

Poe the poet

Simulator

Simulator tasks

Lint tests

Docstrings

What is the purpose of this pull request?

odudex commented Jan 7, 2024 • edited Loading

qlrd commented Jan 8, 2024 • edited Loading

qlrd commented Jan 8, 2024

odudex commented Jan 8, 2024

tadeubas commented Jan 8, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeubas Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeubas Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

qlrd Jan 8, 2024 • 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

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

qlrd commented Jan 8, 2024 • edited Loading

tadeubas commented Jan 8, 2024

qlrd commented Jan 9, 2024 • edited Loading

qlrd commented Jan 10, 2024

qlrd commented Jan 4, 2024 •

edited

Loading

odudex commented Jan 7, 2024 •

edited

Loading

qlrd commented Jan 8, 2024 •

edited

Loading

tadeubas Jan 8, 2024 •

edited

Loading

tadeubas Jan 8, 2024 •

edited

Loading

qlrd Jan 8, 2024 •

edited

Loading

qlrd commented Jan 8, 2024 •

edited

Loading

qlrd commented Jan 9, 2024 •

edited

Loading