Reader programming feature

[gpongelli]

Thanks for this useful library.

This PR is to add Datamatrix label programming feature through a flag, passed from outer methods to the one that really uses it.

@msva please review, thanks 😃

[msva]

Except of the comment in review, PR looks legit and can be merged, if it doesn't break anything and keeps old behaviour for "old" clients

[gpongelli]

changed to DmtxBoolean, waiting for the next release 😃

[gpongelli]

@msva tested today with other barcode generator, reader programming is ok.

[msva]

@msva tested today with other barcode generator, reader programming is ok.

well, but tests (and so default build configuration (where tests are enabled to build)) fails (as there is a change in parameters count in encode function.

Same failure would happen for every program, that links to libdmtx and uses dmtxEncodeDataMatrix function.

So, this means that we need either:

• fix tests, and make new major release, as there would be API breakage.
• try to make a wrapper, or, maybe some kind of function overloading, or even #ifdefs plus build-time configuration (option in CMakeLists.txt) to keep API/ABI stable for old consumers.

I personally would prefer the sesond variant (not sure in which sub-variant), but it would require more work than just "fix tests", tho 🤷
But as I actually more in the role of "maintaner" now, and not a "developer", you can argue for a new major release, and most probably, I'll agree 🤷

[gpongelli]

I run test just now, didn't see them before.

so you would be able to build a version that has new or old signature depending on some configuration flag, right ?
should be better to add a "./configure" parameter (instead of CMakeLists) ?

[msva]

so you would be able to build a version that has new or old signature depending on some configuration flag, right ?

As one of the possible solutions - yes. Assumes, it will define some parameter, that will trigger #ifdef.

should be better to add a "./configure" parameter (instead of CMakeLists) ?

Actually, it would be better, to add it to both, as CMake is considered as primary buildsystem, but you can add it to autotools, if you prefer, and I'll port it to CMake too a after merge.

[msva]

Another variant is to go further, and make C11 (or newer) as minimum C standard to build the library.
And there is kinda possibility for overloading with _Generic

[gpongelli]

so you would be able to build a version that has new or old signature depending on some configuration flag, right ?

As one of the possible solutions - yes. Assumes, it will define some parameter, that will trigger #ifdef.

Yes, this is the idea: a configure optional flag to undef the breaking feature of reader programming.
It translates to #defin / #undef depending on the flg.

should be better to add a "./configure" parameter (instead of CMakeLists) ?

Actually, it would be better, to add it to both, as CMake is considered as primary buildsystem, but you can add it to autotools, if you prefer, and I'll port it to CMake too a after merge.

I do not know CMake and I'm building the library without it, so I cannot help on this task.

[gpongelli]

with latest two commits, it's possible to build the library in following ways:
./configure .... breaking feature enabled by default

./configure .... --disable-reader-programming to disable the feature

with both the configuration, make and make check works.

[jontrulson]

Should a breaking feature be enabled by default?

[gpongelli]

Depends on which version will be released, if project follows semantic versioning: - On a v1.0 , yes (major version increment can add breaking changes) - On a v0.8 surely not (it’s only a minor increment without breaking changes) The point is that, even if it will be called v0.8, if the feature is enabled it will break the backward compatibility, so it’s a non-sense call it v0.8 . Anyway, for now it's enabled by default until a decision from repo owner will be taken on this point, and I’ll eventually do change next days.

…

Il giorno 16 dic 2022, alle ore 18:44, Jon Trulson ***@***.***> ha scritto:  Should a breaking feature be enabled by default? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[gpongelli]

Hi @msva , I've just added a gihub workflow to build and test the project, could you enable actions through project's Settings ?

Then, I would know your point about reader programming feature's default: is it ok for you to have it default enabled ?

Thanks,

[gpongelli]

I've just added a commit which "beautifies" the code, it's all a matter of whitespaces.

I personally would prefer the sesond variant (not sure in which sub-variant), but it would require more work than just "fix tests", tho 🤷 But as I actually more in the role of "maintaner" now, and not a "developer", you can argue for a new major release, and most probably, I'll agree 🤷

I agree to have a new major release number, @msva I'm waiting for it and I'm hoping it will be done very soon, thanks.

[gpongelli]

hi @msva , with recent changes I've:

• aligned the project to have only one point with VERSION declaration
• reverted some "style" changes on dmtx.h file
• added a new method to know if reader programming feature was built (it's a ./configure option, so I thought it can be useful)
• bump to version 1.0.0 due to breaking API when readere programming feature is compiled

I hope you've some time to review this PR, thanks!

[gpongelli]

hi @msva , any news about reviewing this PR ?

let me know if style changes clutter too much the review and if you prefer a different PR for that changes.

Regards,

[msva]

Hi!

Sorry for disappearing (I had some issues).

Talking about this PR: as I see, some critical notes I posted above still actual.

First of all, that programming feature is still enabled-by-default, as I see, while it should be disabled by default, at least for few near versions (at least until major version change) to not break compatibility with applications.

Next, did you performed test with applications, that uses libdmtx, that they don't crash and keep working with modified version, if they know nothing about reader programming, and was linked against non-modified version?

And that they compiles with modified version and works fine afterwards?

As I see, merging the code as is will brake many, if not all, applications that use libdtmx 🤷

[gpongelli]

Hi !

First of all, that programming feature is still enabled-by-default, as I see, while it should be disabled by default, at least for few near versions (at least until major version change) to not break compatibility with applications.

quoting myself:

Depends on which version will be released, if project follows semantic versioning:

• On a v1.0 , yes (major version increment can add breaking changes)
• On a v0.8 surely not (it’s only a minor increment without breaking changes)

I've no idea of which will be next release number or any plan of this project.
Maintainer could merge this PR and make a v1.0, or plan for a v0.8 so I'll change the configure flag.
It depends on how people decide to work on its own project.

Next, did you performed test with applications, that uses libdmtx, that they don't crash and keep working with modified version, if they know nothing about reader programming, and was linked against non-modified version?

I tested it into this package, it works as expected:

• old method signature when feature is disabled -> application that uses libdmtx do not see anything different
• new method signature when feature is enabled
• feature works when reader programming barcode is read by a barcode scanner

And that they compiles with modified version and works fine afterwards?

The feature adds a breaking changes for libsdmtx users, so enabling it means at least to change their code.
You can see this change into test file, where the call is different depending on HAVE_READER_PROGRAMMING build flag.

[msva]

Well, my point is that there is no plans to release 1.x at the moment (or it will require to temporal split to separate branches, and keep supporting old branch and release patch-releases for some time.

So, it is necessary to either you rewrite code a bit to make it disabled by default, or me doing thar after merging (but unfortunately, at the moment I've no enouth spare time even on holidays, so I'd be able to do that myself not earlier than winter.

So, if it is okay to wait few months, I'll merge current state and will fix default state myself.

If you'd like to merge it right now - can you, please, rewrite that part.

Btw, future note: it would be nice to perform style and indentation changes/fixes in separate PRs (from code changes), as they're making reviewing harder.

[msva]

Well, actually, I have "time issues" even furing winter.
And... Well, I can't give any ETA about when I have enough time to finish the review (mostly I'm aware of "code beautifying" commit. It is too hard to make sure it didn't borked up anything) and perform some rewrites to keep this new feature in disabled-by-default state (to don't break backward compatibility).