openfoodfacts-server icon indicating copy to clipboard operation
openfoodfacts-server copied to clipboard

refactor: Moosify ProductOpener::Nutriscore.pm

Open smonff opened this issue 11 months ago • 6 comments

What

This is just an experiment that tries to use Moose in Nutriscore.pm.

Could have been done with Moo or the core Class feature too: no strong opinion about which one should be chosen. We want to try and see what happens, and evaluate the amount of refactoring needed if we make one module an object.

I guess this is not too bad, but a couple of tests still fail.

smonff avatar Mar 24 '24 16:03 smonff

Some tests are failing, this is still work in progress.

smonff avatar Mar 24 '24 16:03 smonff

Codecov Report

Attention: Patch coverage is 75.75758% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 44.27%. Comparing base (dc04d18) to head (0351509). Report is 165 commits behind head on main.

Files Patch % Lines
lib/ProductOpener/Nutriscore.pm 75.00% 8 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10007      +/-   ##
==========================================
- Coverage   49.54%   44.27%   -5.27%     
==========================================
  Files          67       70       +3     
  Lines       20650    20995     +345     
  Branches     4980     5038      +58     
==========================================
- Hits        10231     9296     -935     
- Misses       9131    10525    +1394     
+ Partials     1288     1174     -114     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Mar 24 '24 16:03 codecov-commenter

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Mar 24 '24 17:03 sonarqubecloud[bot]

It would be good to see the equivalent PR using the new Perl core class support. As someone who uses a lot of different languages I find the Moose syntax very difficult to follow.

john-gom avatar Mar 25 '24 17:03 john-gom

It would be good to see the equivalent PR using the new Perl core class support. As someone who uses a lot of different languages I find the Moose syntax very difficult to follow.

This MR is an experiment. The aim is to check if, as a proof of concept, it's possible to use an “object paradigm” with Nutriscore.pm, and have an idea of the effort needed. I have absolutely no opinion about which toolkit should be used (Moo(se), or the core Class feature).

Tried with Moose as a “reasonable enough” fast way to prototype. I don't pretend it should be a final choice.

We identified that the core Class feature would have required an upgrade to Perl 5.38, though. What was maybe slightly too optimistic for a weekend hackathon. A workaround would have been to use Object::Pad (only require 5.26), but it would have introduced more experimental aspects and API instabilities.

Actually, I was more excited experimenting with the core feature, but it looked more appropriate to use Moose after a second thought.

smonff avatar Mar 25 '24 20:03 smonff

It would be good to see the equivalent PR using the new Perl core class support. As someone who uses a lot of different languages I find the Moose syntax very difficult to follow.

Hi John,

I guess your comment is a general statement about Moose in the Perl ecosystem. OK, it's a matter of taste, I won't argue about that. But in the particular case of this PR, there is very little that is specific to Moose. Clients of Nutriscore.pm must use method calls instead of subroutine calls : this would be the same with any other OO system. Same thing for internal method within the module. The only difference between Moose and Corinna would be in the 15 lines that declare attributes for nutriscore objects.

Regarding the new Corinna OO system shipped with perl 5.38, I think it's a very nice addition to the Perl language, but aiming at short-term usage of Corinna in a heavily loaded production system would be risky. Corinna is advertised as being experimental, the implementation is not optimized yet, the syntax might change in future versions, and currently you must explicitly state "use feature 'class'" and then also add "no warnings 'experimental'" !

I know it's a kind of chicken-and-egg problem. If an important project like openfoodfacts chooses to go for Corinna, that would be a big boost for acceptance of the new feature within the Perl community. But it's also a big risk to take for the openfoodfacts project.

By contrast, Moose is very well established in the Perl community, many CPAN modules depend on it, and it offers many more features than current Corinna (for example the 'lazy' attribute used by smonff in his POC -- a mechanism very relevant for openfoodfacts, considering the fact that some computations are expensive).

damil avatar Mar 26 '24 21:03 damil