Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Device dependent margins/config #54
Device dependent margins/config #54
Changes from 5 commits
d73626d
06f61ea
3aecdc7
1872e40
1b279b3
2353512
dcc6234
040f2c2
f907b8b
d64c977
a7def5e
d8dcb3d
4fc8b68
1d36049
d8b3b6d
13fa9b9
424ec34
d342a04
1786928
16d82e8
e239867
b390217
0bdf8fd
a4ed930
fdf3538
0fa7ad5
7246473
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 this really needed? In case the list is constant, it would be better as a convention to prefer tuple over list
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.
The amount of supported tape sizes varies from printer to printer. I got an error that I tried to change the "tuple" size. A quick google search gave this as a solution. If there is a better solution please elaborate.
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.
Ah, tuples are "immutable" so you should always generate a new one instead of modifying an old one.
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 didn´t complain this time)
Edit:
Sorry, had to revert this:
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.
Using
append
is a dynamic operation. I think it would look better to doThere 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.
I'll try (the append was just copied from an example)
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.
I think that eventually we should move these out of the code, into a JSON file. If possible, please try to do it. In not, we can try refactor it later.
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.
I think we need a proper data model(1) for device models(2). (The first "model" in that sentence means "implementation of a schema" while the second "model" refers to a particular product that is sold by a company.)
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.
Sorry, I was a bit confused You do already define
DeviceConfig
.I'm wondering how to achieve this elegantly. I agree that it's in principle better to define these things out-of-code like in JSON. However, that unfortunately would require deserialization. Pydantic is a popular library for this, but then we're adding more project dependencies and a lot of complexity, also in the code.
The way I see it, we have two main options:
NamedTuple
ordataclass
.Another disadvantage to 1. is that editing the config would involve editing serialized data which can be a real pain. An advantage to 2. is that code tools like mypy will automatically perform validation for the data-as-code.
I'm leaning more towards 2.
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.
What I was trying to achieve here is a what in C would be "array of structs" for the "const" configuration.
If you have any suggestions how to do this elegantly in python, let me know...
My idea was that the (existing) labeler class took this as input to know the device's exact properties.
I agree that having this out of code would be nice. But can we please keep this contained?
I started out with "just adding my printer", and it is getting a bit out of hand already imho.
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.
Converted DeviceConfig to a dataclass
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.
It's hard to understand from reading this what each field means. Please explicitly name each field.
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.
To be explicit, this is as simple as
Also, the use here of
SUPPORTED_DEVICE_ID
seems very indirect to me. Why not just doThere 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.
If that is possible can you give me a syntax example? (It would definitely be a lot clearer)
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.
@FaBjE that was an explicit example, and should work without other modifications to your code if you continue the obvious pattern. (Python lets you specify the names of positional arguments in the function invocation.) Does this make sense?
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.
I didn´t see your example at first. I had this page already open I guess.
But thanks! I will try to add.
I added the enum because before the DeviceConfig class I checked with if-else in the code which properties applied to the printer (Based on the device ID), but that was ugly imho.
I agree it is kind of redundant now. Shall I remove it?
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.
Here's an example:
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
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.
Please add a link here to a markdown document under
/doc
which describes what the calibration routine is.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.
Will check
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.
Unfortunately there is no document at this moment. I think we need to add a link when it is there.
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.
Maybe define the class as
@dataclass
. https://docs.python.org/3/library/dataclasses.htmlThere 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.
Personally, I'd probably use a
NamedTuple
which is simpler, but either should work.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.
I'm not sure of the pro's and con's of both. But I think dataclass is easier to convert to?
I'll give it a shot
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.
Made dataclass
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.
I am not sure a function is really needed for this, as it is rather short, and can be replaced with a simple expression. In any case, it should be refactored as follows:
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.
Will fix. It seemed more "contained" to me than just accessing the array itself from outside the object.
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
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.
Also here,
tape_size_mm
, nottapeSizeMM
(I will refrain now from commenting of improper casing).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.
Please do 😉 I'll do a re-check of my work.
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.
Outdated/fixed
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.
or even 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.
Thanks, will fix. (these are the kind of things you have no clue about when using a new language)
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.
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.
As coding convention, instead of:
do:
This reduces nesting.
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.
Will fix.
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 (now in dymo_labeler.py)
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.
Remove
is True
. It will work the sameThere 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.
ok, will fix
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
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.
As before, style comment. Please use the following convention:
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.
Will fix
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
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.
Why do you return
None
from the function and raise here, instead of raising in the function itself, and then you will not have to test returned value from the function (and change its type to also supportNone
).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.
Good question...
Stupid answer: I learned about the raising exceptions later on.
will fix.
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