makerspace / memberbooth

Displaying member info on a public machine
2 stars 0 forks source link

Implemented label for 3D-printer marker #94

Closed InMyOrbit closed 2 years ago

InMyOrbit commented 3 years ago

Note that it is unknown where the printer cuts the label, this implementation might have to be adjusted after testing it on a printer.

emanuelen5 commented 3 years ago

Don't forget to generate an image of the label in the CI scripts (by adding it to the file .github/workflows/pythonapp.yml)

emanuelen5 commented 3 years ago

I haven't tested it though

emanuelen5 commented 3 years ago

I have now tested it and it works fine! It would also be nice to add a regression test that makes sure that the dimensions of the label stays as intended.

emanuelen5 commented 3 years ago
emanuelen5 commented 3 years ago

I added the Github workflow in the commits above ^

InMyOrbit commented 3 years ago

I have now tested it and it works fine! It would also be nice to add a regression test that makes sure that the dimensions of the label stays as intended.

Added unit test for size. Should we enhance test by creating X random strings representing names and test with them as well?

InMyOrbit commented 3 years ago

We still have to test this on the actual printer, we have not yet confirmed that the height is actually 25 mm.

emanuelen5 commented 3 years ago

I have now tested it and it works fine! It would also be nice to add a regression test that makes sure that the dimensions of the label stays as intended.

Added unit test for size. Should we enhance test by creating X random strings representing names and test with them as well?

If you suspect that might be a failure mode, sure. However, if the font has the same spacing for each letter, then it would suffice to create a test with the maximum allowed name length, to make sure that it will also fit.

emanuelen5 commented 3 years ago

We still have to test this on the actual printer, we have not yet confirmed that the height is actually 25 mm.

InMyOrbit commented 2 years ago

I do agree with what you pointed out, but it is not directly related to this change so I suggest that we make it another issue instead.