search

RamyTalal / Label-Printer

Easily print labels with a Brother label printer.
MIT License
65 stars 22 forks source link

Add barcodes feature #22

Closed xelan closed 2 years ago

xelan commented 2 years ago
Q A
Branch? master
Bug fix? yes
New feature? yes
Deprecations? no
Tickets Fix #8
License MIT
RamyTalal commented 2 years ago

Thank you for your PR! Could you reformat Barcode.php so that the style is aligned with the other files?

RamyTalal commented 2 years ago

I'm also missing the tests.

xelan commented 2 years ago

Sure, I'll update the CS for Barcode.php and add some tests :+1: Thanks for the quick feedback and best regards from Austria

xelan commented 2 years ago

Hi @RamyTalal! I've added tests, removed unused code (the value objects are currently unused, would be nicer in the future but not needed right now). Also we adapted and tested QR code printing (plus tested them on a Brother QL-580N).

Should I squash the commits before merge or would you prefer to keep the work history as-is?

Thanks and best regards Andreas

RamyTalal commented 2 years ago

Hello @xelan Thanks again for the new changes. What do you mean with the unused value objects?

Squash is fine :)

xelan commented 2 years ago

In the first commit of the feature branch for this PR, we had several value objects to encapsulate the constant used in the Barcode command class, however they are not used at the moment so I removed them again.

Ok, then I'll squash the commits and give you an update afterwards :+1:

xelan commented 2 years ago

Squashed and the updated branch is now pushed :)

xelan commented 2 years ago

By the way, the QR code printing, which is marked as TODO in the readme is now implemented (and tested on a QL-580N). If you want, you can also test it and remove it from the TODO if everything works for your label printer as well. And maybe we should add a short readme entry for barcode printing as well?

RamyTalal commented 2 years ago

@xelan I've tested the code and it seems to work fine! Can you add the Readme entry? When that's done I can tag a new release. :)

xelan commented 2 years ago

Added, thanks :+1:

RamyTalal commented 2 years ago

Thanks again for the PR!

xelan commented 2 years ago

Thanks again for the PR!

Thanks for the awesome library and good maintenance! Shouldn't the new version number be bumped to 1.1.0, as it contains new, backwards compatible features?