add new basic00 implementation #1040

hannaeko · 2022-02-21T15:42:41Z

Purpose

Implement new Basic00. I implemented the updated "test case" as sanitization methods, it is not used anywhere yet, I am waiting for design validation before using it.

Context

zonemaster/zonemaster#942

Changes

Add Zonemaster::Engine::Normalization::normalize_name that follow the procedure described in RequirementsAndNormalizationOfDomainNames.
- The method return a list of error and the normalized domain name if it is valid.
- If the domain is invalid, the domain is undef, if it is valid the list of error is empty.
- Errors are object of type Zonemaster::Engine::Normalization::Errors. Those have a tag and a localized error message (translated when called to allow changes of the locale between calls)
Documentation (waiting for design validation)

How to test this PR

A new test file "t/normalization.t" is testing the method.

mattias-p

In general this looks good to me but I have a bunch of comments.

lib/Zonemaster/Engine/Sanitization.pm

t/sanitization.t

lib/Zonemaster/Engine/Sanitization/Errors.pm

fix tests

matsduf

Please add POD documentation of all methods that explains what they do (what they are supposed to do), what the expected input is and what the output will be.

lib/Zonemaster/Engine/Sanitization.pm

lib/Zonemaster/Engine/Sanitization/Errors.pm

lib/Zonemaster/Engine/Sanitization.pm

t/sanitization.t

lib/Zonemaster/Engine/Sanitization.pm

t/sanitization.t

hannaeko · 2022-09-21T16:46:42Z

I have added the NFC conversion tests and updated the documentation.

matsduf

This looks fine. I have not run any tests on it though.

tgreenx

LGTM, tested and it works as expected.
One thing though, overall I think the code could benefit from an editorial pass on the spacing in every conditional blocks (e.g. if ( ... ) everywhere) to improve readability but I won't be blocking the PR if it is not done.

tgreenx · 2022-09-22T12:36:49Z

lib/Zonemaster/Engine/Normalization/Error.pm

+
+=item string
+
+Returns a string representation of the error object, equivalent to message.


error object, equivalent to message. -> error object. Equivalent to message().

tgreenx · 2022-09-22T12:37:01Z

lib/Zonemaster/Engine/Normalization/Error.pm

+
+=item tag
+
+Returns the message tag asscociated to the error.


asscociated -> associated

ghost

lgtm

ghost · 2022-11-04T09:21:29Z

lib/Zonemaster/Engine/Normalization.pm

+use Readonly;
+use Try::Tiny;
+use Zonemaster::LDNS;
+use Data::Dumper;


Might be safely removed

tgreenx

One typo else ready to approve

tgreenx · 2022-11-23T10:17:48Z

lib/Zonemaster/Engine/Normalization/Error.pm

@@ -129,7 +129,7 @@ sub tag {

 =item string

-Returns a string representation of the error object, equivalent to message.
+Returns a string representation of the error object. Euivalent to message().


Euivalent -> Equivalent

matsduf · 2022-12-13T16:15:59Z

Updated by #1157

add new basic00 implementation

c307996

hannaeko added this to the v2022.1 milestone Feb 21, 2022

hannaeko requested review from mattias-p, matsduf, a user and tgreenx February 21, 2022 15:42

fix tests

94ce900

mattias-p reviewed Feb 23, 2022

View reviewed changes

hannaeko added 3 commits March 1, 2022 17:02

make globals readonly

ede669a

assume input is decoded

73e8c8e

fix tests

swap use and extends

cb40bd8

hannaeko force-pushed the implement-new-basic00 branch from b577b68 to cb40bd8 Compare March 1, 2022 16:08

hannaeko added 4 commits March 1, 2022 17:12

remove debugging

ff4a134

use standard class accessor interface

c2a2791

refactor sanitize_label

9a2f429

prefer single method

84adfdd

matsduf reviewed Mar 28, 2022

View reviewed changes

lib/Zonemaster/Engine/Sanitization.pm Outdated Show resolved Hide resolved

lib/Zonemaster/Engine/Sanitization/Errors.pm Outdated Show resolved Hide resolved

lib/Zonemaster/Engine/Sanitization.pm Outdated Show resolved Hide resolved

t/sanitization.t Outdated Show resolved Hide resolved

matsduf mentioned this pull request Apr 4, 2022

Makes solidus valid character in ASCII labels zonemaster/zonemaster-backend#969

Merged

hannaeko added 3 commits April 7, 2022 10:54

update error messages

f7bf3c1

use utf8 in sanitization files

b00402d

rename accessor

a1331cf

matsduf requested changes Apr 8, 2022

View reviewed changes

lib/Zonemaster/Engine/Sanitization.pm Outdated Show resolved Hide resolved

t/sanitization.t Outdated Show resolved Hide resolved

matsduf modified the milestones: v2022.1, v2022.2 May 4, 2022

use returned error instead of exception

784514f

hannaeko mentioned this pull request Aug 19, 2022

Convert Basic00 to be a general specification zonemaster/zonemaster#942

Merged

trim whitespaces

a4208c6

hannaeko marked this pull request as draft August 19, 2022 14:04

rename module to better match spec

91832c0

hannaeko added 4 commits September 21, 2022 18:32

update pod documentation for normalization module

d8a8162

add test for nfc conversion

35f9c4b

add new files to manifest

0d54295

rename normalization error module

61f5d9b

matsduf previously approved these changes Sep 21, 2022

View reviewed changes

tgreenx reviewed Sep 22, 2022

View reviewed changes

tgreenx linked an issue Nov 3, 2022 that may be closed by this pull request

Undelegated tests crash when the domain has a trailing dot #965

Closed

ghost previously approved these changes Nov 4, 2022

View reviewed changes

hannaeko added 2 commits November 21, 2022 11:58

fix coding style

a354e4c

fix typo in doc

f9095e1

hannaeko dismissed stale reviews from ghost and matsduf via f9095e1 November 21, 2022 10:59

hannaeko added the V-Minor Versioning: The change gives an update of minor in version. label Nov 21, 2022

matsduf previously approved these changes Nov 21, 2022

View reviewed changes

tgreenx reviewed Nov 23, 2022

View reviewed changes

fix typo

9b248a6

hannaeko dismissed matsduf’s stale review via 9b248a6 November 24, 2022 10:34

tgreenx requested review from a user, tgreenx and matsduf November 24, 2022 13:46

tgreenx approved these changes Nov 24, 2022

View reviewed changes

matsduf approved these changes Nov 24, 2022

View reviewed changes

hannaeko merged commit e224292 into zonemaster:develop Nov 28, 2022

tgreenx mentioned this pull request Jun 14, 2023

Normalize zone name before test start #739

Closed

hannaeko mentioned this pull request Oct 24, 2023

use name normalization method zonemaster/zonemaster-backend#1132

Merged

This was referenced Nov 29, 2023

Remove Basic00 Test Case zonemaster/zonemaster#1226

Merged

Remove Basic00 implementation #1308

Merged

tgreenx mentioned this pull request Sep 17, 2024

Undelegated tests crash when the domain has a trailing dot #965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new basic00 implementation #1040

add new basic00 implementation #1040

hannaeko commented Feb 21, 2022 •

edited

Loading

mattias-p left a comment

matsduf left a comment

hannaeko commented Sep 21, 2022 •

edited

Loading

matsduf left a comment

tgreenx left a comment •

edited

Loading

tgreenx Sep 22, 2022

hannaeko Nov 21, 2022

tgreenx Sep 22, 2022

hannaeko Nov 21, 2022

ghost left a comment

ghost Nov 4, 2022

hannaeko Nov 21, 2022

tgreenx left a comment

tgreenx Nov 23, 2022

hannaeko Nov 24, 2022

matsduf commented Dec 13, 2022


		=item string

		Returns a string representation of the error object, equivalent to message.

add new basic00 implementation #1040

add new basic00 implementation #1040

Conversation

hannaeko commented Feb 21, 2022 • edited Loading

Purpose

Context

Changes

How to test this PR

mattias-p left a comment

Choose a reason for hiding this comment

matsduf left a comment

Choose a reason for hiding this comment

hannaeko commented Sep 21, 2022 • edited Loading

matsduf left a comment

Choose a reason for hiding this comment

tgreenx left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Dec 13, 2022

hannaeko commented Feb 21, 2022 •

edited

Loading

hannaeko commented Sep 21, 2022 •

edited

Loading

tgreenx left a comment •

edited

Loading