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

Define argument name "label" and "unicode_name" #1126

Merged
merged 3 commits into from
Sep 13, 2022

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented Aug 31, 2022

Purpose

Both "dlabel" and "label" are used as argument names, but none of them are defined. The former is used only once, the latter is clearer and used several times. With this PR "label" is defined in the documentation and "dlabel" is not used any more.

To make implementation consistent, "dlabel" is replaced by "label" in the affected Perl module and in the PO files.

In addition, the argument name "unicode_name" is defined (not yet in use in any code).

Context

In zonemaster/zonemaster#942 (comment) it was noted that undefined "dlabel" was used in that PR. Instead of defining that, it will be replaced by "label" that this PR defines.

Changes

  • docs/logentry_args.md
  • lib/Zonemaster/Engine/Test/Basic.pm
  • share/da.po
  • share/es.po
  • share/fi.po
  • share/fr.po
  • share/nb.po
  • share/sv.po

How to test this PR

The main change is documentation change only. Review the change and compare with the implementation.

The other change affects the PO files. That can be tested by following the "Instructions for translators":

  1. Update the PO files by running ./update-po da.po and for all other PO files.
  2. Check the arguments by running ../util/check-msg-args da.po and for all other PO files.

tgreenx
tgreenx previously approved these changes Sep 1, 2022
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

LGTM.

ghost
ghost previously approved these changes Sep 1, 2022
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Beside a suggestion (not necessarily related to the PR), this looks ok.

@@ -91,7 +92,6 @@ defined *arguments*.
|| DNS packet size| The size in octets of a DNS packets.|
|| DNSKEY key length| The key length for a DNSKEY. The interpretation of this value various quite a bit with the algorithm. Be careful when using it for algorithms that aren't RSA-based.|
|| DNSSEC delegation verification failure reason| A somewhat human-readable reason why the delegation step between the tested zone and its parent is not secure.|
| dlabel (?) | Domain name label| A single label from a domain name.|
| dlength (?) | Domain name label length| The length of a domain name label.|
Copy link

Choose a reason for hiding this comment

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

It seems the dlength is used, have you considered moving it to the Defined arguments table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did consider that too, but I could not make up my mind if it should be "length" (since all lenght I know of is a positive integer) and defined as "Length of some entity" or "labellength"? If just "length" then it could be used in several cases.

In the same message ID there is also a "max". Should that be "max", "maxlength" or "maxlabellenght"?

There are several argument names to define.

Copy link

Choose a reason for hiding this comment

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

I don't know either, but if we concatenate several words, I'd suggest to stick to snake case as already done for other labels. Hence it could be "label_length".

@matsduf matsduf dismissed stale reviews from ghost and tgreenx via 73c21d9 September 2, 2022 20:27
@matsduf matsduf changed the title Define argument name "label" Define argument name "label" and "unicode_name" Sep 2, 2022
@matsduf matsduf requested review from a user and tgreenx September 2, 2022 20:28
@@ -70,7 +71,7 @@ and updated messages (*msgids* and *msgstr*).
| rcode | An RCODE Name | An RCODE Name (not numeric code) from [DNS RCODEs]. |
| rrtype | A Resource Record TYPE Name | A Resource Record TYPE Name (not numeric code) from [DNS RR TYPEs]. |
| testcase | A Zonemaster test case, or `all` | A test case identifier. |

| unicode_name | Unicode name of a code point | The name is a string in ASCII only and in upper case, e.g. "LATIN SMALL LETTER A"|
Copy link

Choose a reason for hiding this comment

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

In addition, the argument name "unicode_name" is defined (not yet in use in any code).

Do you have an example of how this argument would be used?

Copy link

Choose a reason for hiding this comment

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

@matsduf matsduf merged commit 3f58c8a into zonemaster:develop Sep 13, 2022
@matsduf matsduf deleted the update-argument-definition branch September 13, 2022 06:41
@matsduf matsduf added the V-Patch Versioning: The change gives an update of patch in version. label Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants