rscada / libmbus

Meter-bus library and utility programs
http://www.rscada.se/libmbus
BSD 3-Clause "New" or "Revised" License
229 stars 142 forks source link

WIP: Add json methods to library #132

Closed Apollon77 closed 6 years ago

Apollon77 commented 6 years ago

Hi,

I currently adapt a nodejs native module to introduct libmbus into nodejs and therefor I base on the great work of https://github.com/samkrew/libmbus . I can not reach samkrew and also he did not updated the underlaying libmbus repo sometime, but verified his work after re-applying his changes on the current state of the libmbus github repo.

So I would like to bring in this pull request to add json output methods into the library.

If I should change anything please tell me what and I will be happy to do so.

Greetings,

Ingo

Apollon77 commented 6 years ago

I added one commit to correctly return ISO conform timestamps with "Z" at the end

lategoodbye commented 6 years ago

Hi Ingo, sorry for my late reply and thank for you contribution. I see two reason why i can't merge this request. First reason is that the change only apply to the old API mbus-protocol.c and not mbus-protocol-aux.c. Second reason is that i don't want to add any additional output formats which we need to maintain. How about converting libmbus XML output to JSON (xlst)?

Nevertheless the commit 29a9e5a looks great, please make a separate pull request i will merge it ASAP. It would be nice to have the timestamps in mbus-protocol-aux.c in the same format, too.

Stefan

Apollon77 commented 6 years ago

Hi Stefan,

The reason would mainly be performance and also that JSON is a common standard in the meantime and easier to work with then XML, but I understand you basically.

Timestamps: Will see what I can do.

Apollon77 commented 6 years ago

Hi Stefan,

I removed the json methods, still in here are:

Please have another review and tell me your needs to accept the change. Then I can create a new clean PR :-)

lategoodbye commented 6 years ago

I'm fine with the timestamp changes and there is no problem with the whitespace fixes.

Apollon77 commented 6 years ago

So build/configure stuff should be removed?!

lategoodbye commented 6 years ago

Yes, these are unrelated and even without a helpful commit message.

Apollon77 commented 6 years ago

The build and configure changes came from https://github.com/samkrew/libmbus/commit/b1ad2f6100114c0cbbee88763be25073560109c7 ... I try to revert then and see if ci still builds.

BTW: could you also please have a look at #133?! ;-)

Apollon77 commented 6 years ago

Ok, after reverting changes to configure/build the compile using gcc on macOS failed: https://travis-ci.org/Apollon77/libmbus/jobs/352707224

After adding back one "autoreconf" in build.sh it works again ... so for macos this change seems to be needed. https://travis-ci.org/Apollon77/libmbus/jobs/352709971

With this reason: is this change acceptable?!

lategoodbye commented 6 years ago

As i said before without a self-explaining commit message i cannot accept such changes. I confess some of my old commit aren't always better, but in case of a regression reverting is much easier to follow.

lategoodbye commented 6 years ago

Should i make a suggestion for the commit message?

Apollon77 commented 6 years ago

No, everything good. Had no time to follow up this week. And (because bound a bit somehow) a firt feedback to #133 would be also helpful that I know how to proceed. Reason is that the PR from #133 is based on #132 in my fork, so when doing the needed reset and re-commits for #132 I also need to rebase 133 and such ...

lategoodbye commented 6 years ago

That's bad. Your pull requests must be independent. As i said before please make a fresh, separate pull request for the fixes (with proper commit messages and without reverts).

Apollon77 commented 6 years ago

I know ... I will fix that soon

lategoodbye commented 6 years ago

What's the state of the timestamp changes?

Apollon77 commented 6 years ago

As soon as the nodejs module now ist stable I will start reset my fork to your codebase and clean up anything and then send you clean PRs for several things ... And after this I will re-do the windows branch ... give me some days :-) I need to get a clean base for this first

Apollon77 commented 6 years ago

@lategoodbye You will get 3 new PRs later today (CI is currently taking long :-( ) then I close this one here

Apollon77 commented 6 years ago

I still try to reproduce the macos build problem. Currently it works also without the one "autoreconfigure" line, but I also had issues in some build tries ... I do not understand it. As soon as I can reproduce it I will open a new issue/pr