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

Remove the limit of unnamed template argument number #277

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

xxyzz
Copy link
Collaborator

@xxyzz xxyzz commented Apr 25, 2024

0 and above 1000 are allowed in MediaWiki, for example, fr edition's "cardinaux/vi" template:
https://fr.wiktionary.org/wiki/Modèle:cardinaux/vi

0 and above 1000 are allowed in MediaWiki, for example, fr edition's
"cardinaux/vi" template:
https://fr.wiktionary.org/wiki/Modèle:cardinaux/vi
Copy link
Collaborator

@kristian-clausal kristian-clausal left a comment

Choose a reason for hiding this comment

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

Those are some big numbers. This seems obviously correct to me, and if the numbers go that high I bet the limit is actually something like int, in which case it doesn't really make sense to check upper limits.

However, we might need to check for lower, so there should still be a k < 1 test. In that case you might as well leave the deleted block and replace the magic number 1000 with MAX_TEMPLATE_KEY_VALUE_INT or something similar, and make the constant the size of an unsigned(?) int or whatever makes most sense... If there is anything that makes the most sense. PHP doesn't seem to have unsigned ints, but it does have different sizes for 32 and 64 bit... Yeah, I guess at this point it doesn't make sense to check max, just k < 1.

@xxyzz
Copy link
Collaborator Author

xxyzz commented Apr 25, 2024

0 is also allowed and str.isdigit() returns False for negative string number so I think we don't need more checks.

@kristian-clausal
Copy link
Collaborator

Then everything seems correct.

@xxyzz xxyzz merged commit 21a9316 into tatuylonen:main Apr 25, 2024
5 checks passed
@xxyzz xxyzz deleted the args branch April 25, 2024 05:17
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