wc-duck / datalibrary

Open Source Data Library for data serialization.
Other
42 stars 8 forks source link

Fix for bitfield defaults #73

Closed lundmark closed 6 years ago

lundmark commented 6 years ago

Previously, having defaults for bitfields did not yield the correct result. This was because bitfields can be sub-byte sized and previous implementation of defaults did not take this into consideration.

I'm not 100% certain that I've done everything correct so please review the code in dl_txt_pack before admitting the pull request.

lundmark commented 6 years ago

Also... is it possible to turn off whitespace diffs in github? This shit be craycray.

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 76744f5cc6f10114be25e4bd745ce089949c5ec9 on lundmark:master into on wc-duck:master.

lundmark commented 6 years ago

This is failing because the build is already failing on these points.

wc-duck commented 6 years ago

Except for the "to big default"-check this looks fine! I'll merge when that check is moved.

lundmark commented 6 years ago

Awesome! What's wrong with the too big default value?

wc-duck commented 6 years ago

I think the check should be done when parsing the typelib, not when packing in instance... Or did I miss something when reading the changes on my phone and a train? :)

lundmark commented 6 years ago

No you are entirely correct. Interestingly enough, that already exists!

Should I check for too big value though?

lundmark commented 6 years ago

That is already there as well! I'll just remove the check and commit!

lundmark commented 6 years ago

As you can see I added unittesting for using true/false values instead of 1/0 on bitfields. I think this is a really good thing because it's something that our content creators think is really important.

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling c86445d186149654f404c9e74c92f4170624fb1c on lundmark:master into on wc-duck:master.

wc-duck commented 6 years ago

Can't merge on my phone but consider this merged an closed!

lundmark commented 6 years ago

Apparently I can do it myself? Thanks for the trust!