Skip to content

Fix Zonemaster::Engine::Translator’s instance() method#1347

Merged
marc-vanderwal merged 2 commits intozonemaster:developfrom
marc-vanderwal:bugfix/misc-translator-bugs
Jun 3, 2024
Merged

Fix Zonemaster::Engine::Translator’s instance() method#1347
marc-vanderwal merged 2 commits intozonemaster:developfrom
marc-vanderwal:bugfix/misc-translator-bugs

Conversation

@marc-vanderwal
Copy link
Contributor

@marc-vanderwal marc-vanderwal commented May 15, 2024

Purpose

This PR fixes a bug in Zonemaster::Engine::Translator that causes problems in Zonemaster::Backend::Translator (in Backend, not Engine), which inherits from Zonemaster::Engine::Translator.

Context

While adding unit tests in Zonemaster::Backend for its Translator module, I noticed that the instance() method in Zonemaster::Engine::Translator always returned undef instead of a Translator object.

Changes

  • Fix a bug in the instance() method which prevented it from working as documented.
  • Fix small mistakes in the module’s documentation.
  • Change unit test so that it calls instance() instead of new(), to exercise the code.

How to test this PR

Run the test suite in Zonemaster::Engine. Expect all unit tests to pass.

A programming error in Zonemaster::Engine::Translator’s instance()
method caused the global variable $instance to be immediately undefined
right after the initialize() function set it to an instance of a
Translator object, if that singleton object hadn’t been previously
instanciated.

The documentation for Zonemaster::Engine::Translator’s initialize()
method was also incorrect: this function takes a raw list of arguments,
not a hash reference.

Do not use the new() method in the corresponding unit tests anymore,
because that method is deprecated.
@marc-vanderwal marc-vanderwal added T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version. labels May 15, 2024
@marc-vanderwal marc-vanderwal added this to the v2024.1 milestone May 15, 2024
@marc-vanderwal
Copy link
Contributor Author

I’m moving one commit from this PR into its neighbor #1346, because it’s more relevant there than here. The idea is to make #1346 address the most urgent issues in Zonemaster::Engine::Translator and this one the more latent problems.

The documentation for Zonemaster::Engine::Translator still referred to
Moose in one place.
@marc-vanderwal marc-vanderwal force-pushed the bugfix/misc-translator-bugs branch from b8895fe to c65eb5b Compare May 15, 2024 14:07
@marc-vanderwal
Copy link
Contributor Author

I’m keeping this PR as a draft for now, because I’m considering also deprecating the new() method altogether (as is stated in the documentation). A proper singleton class ought to have a private constructor and only expose the instance() function. If so, I’ll need to remember to change the V-Patch tag to V-Major.

@marc-vanderwal marc-vanderwal changed the title Fix multiple regressions in Engine’s and Backend’s Translator modules Fix Zonemaster::Engine::Translator’s instance() method May 16, 2024
@marc-vanderwal
Copy link
Contributor Author

I’m keeping this PR as a draft for now, because I’m considering also deprecating the new() method altogether (as is stated in the documentation). A proper singleton class ought to have a private constructor and only expose the instance() function. If so, I’ll need to remember to change the V-Patch tag to V-Major.

That’s better done in another PR.

@marc-vanderwal marc-vanderwal marked this pull request as ready for review May 16, 2024 07:30
@matsduf matsduf added the P-High Priority: Issue to be solved before other label May 22, 2024
@marc-vanderwal marc-vanderwal merged commit 8bdc426 into zonemaster:develop Jun 3, 2024
marc-vanderwal added a commit to marc-vanderwal/zonemaster-backend that referenced this pull request Jun 3, 2024
Pull request zonemaster-engine#1347
(zonemaster/zonemaster-engine#1347) has been
merged, which fixes Zonemaster::Backend::Translator->instance(). So we
can remove one use of the deprecated
Zonemaster::Backend::Translator->new() in the unit tests.
@marc-vanderwal marc-vanderwal deleted the bugfix/misc-translator-bugs branch June 12, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P-High Priority: Issue to be solved before other T-Bug Type: Bug in software or error in test case description 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.

3 participants