Skip to content

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

Merged
matsduf merged 3 commits into
zonemaster:developfrom
matsduf:update-argument-definition
Sep 13, 2022
Merged

Define argument name "label" and "unicode_name"#1126
matsduf merged 3 commits into
zonemaster:developfrom
matsduf:update-argument-definition

Conversation

@matsduf

@matsduf matsduf commented Aug 31, 2022

Copy link
Copy Markdown
Contributor

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

@tgreenx tgreenx left a comment

Copy link
Copy Markdown
Contributor

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

@ghost ghost left a comment

Copy link
Copy Markdown

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.

Comment thread docs/logentry_args.md
|| 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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Comment thread docs/logentry_args.md
| 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
Copy Markdown

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
Copy Markdown

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