Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a format parameter to the Decimal type #715

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saggi-dw
Copy link
Contributor

@saggi-dw saggi-dw commented May 8, 2024

A user on DokuWiki Forum need a special format for numbers.
https://forum.dokuwiki.org/d/22218-how-to-force-leading-zeroes-when-using-a-bureacuracy-form-with-struct

This add a new config parameter for the Decimal type: format

If format is set and valid all other formatting parameters(roundto, decpoint, thousands, trimzeros, engineering) are ignored.
format must be a valid format for sprintf and accepts the following number formats: bdeEfFu

@splitbrain
Copy link
Member

LGTM. Can you fix the code sniffer issue and squash your commits?

@saggi-dw
Copy link
Contributor Author

saggi-dw commented May 8, 2024

I hope that was all correct. My first question: What is squashing? 🙈
I think it worked.

@splitbrain
Copy link
Member

Great. One more thing: can you please extend the unit tests to cover your new setting? See https://github.com/cosmocode/dokuwiki-plugin-struct/blob/master/_test/types/DecimalTest.php

@saggi-dw
Copy link
Contributor Author

OK, test failed:
There was 1 failure:

1) dokuwiki\plugin\struct\test\types\DecimalTest::test_renderValue with data set #43 ('1', '1 ', '-1', '.', ' ', true, '', '', false, '%u')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1 '
+'1'

I will fix that.

@saggi-dw
Copy link
Contributor Author

The commit message is now total crap. Unfortunately, I have no idea whether it can still be changed...

@splitbrain
Copy link
Member

The commit message is now total crap. Unfortunately, I have no idea whether it can still be changed...

You can do a git commit --amend it lets you edit the last commit. Once done, do another force push.

['362525200', '3.625e+8' , '-1', '.', ' ', true, '', '', false, '%.3e'],
['362525200', '3.625E+8' , '-1', '.', ' ', true, '', '', false, '%.3E'],
['1', '1' , '-1', '.', ' ', true, '', '', false, '%u'],
['-1', '18446744073709551615' , '-1', '.', ' ', true, '', '', false, '%u'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add tests for where the format string is wrong and thus ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. The - should be in the regex as a character and not as a range. By extending the test I realized that. Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last test looks interesting. I think it is dependent on the platform and would fail on a 32bit system

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course you're right again!
32bit:

php > printf('%u','-1');
4294967295

I will remove the last line of the test and adjust the penultimate one. It should only be checked whether %u is recognized.

refine regex and extend test
remove test of negative value for %u
adjust test for %u

fix %s -> %u
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants