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

refactor: Moosify ProductOpener::Nutriscore.pm #10007

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

smonff
Copy link
Contributor

@smonff smonff commented Mar 24, 2024

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.

This is just an experiment. Could have been done with Moo too.
@smonff
Copy link
Contributor Author

smonff commented Mar 24, 2024

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

@codecov-commenter
Copy link

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 ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented Mar 24, 2024

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

@john-gom
Copy link
Contributor

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.

@smonff
Copy link
Contributor Author

smonff commented Mar 25, 2024

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.

@github-actions github-actions bot added the ⭐ top pull request Top pull request. label Mar 26, 2024
@damil
Copy link
Contributor

damil commented Mar 26, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants