Skip to content

Conversation

@bdraco
Copy link
Member

@bdraco bdraco commented Mar 18, 2025

TLDR: Setting __config_file__ and __line__ on the cython object meant they were not always readable in pure python. This turned out to be a corner case with dunder names.

Cythonizing the Node*Class objects proved to be unneeded anyways since the cost of small functions is better optimized by cythonizing the constructors. Make sure they are set using the object protocol instead since there is no concern about if it will only work in Cython.

Summary of changes

  • Drop cythonizing on objects.py since it caused the issue above
  • Collapse the three different _add_reference.. calls in reference.py since its all the same to _add_reference_to_node_class now that there is no value in cythonizing them. Add a check and test for start_mark being None
  • Move _handle_mapping_tag, _construct_seq, and _handle_scalar_tag to constructors.py so they can be cythonized instead - no changes to these functions other than calls to _add_reference_to_node_class
  • Move _add_reference to reference_object.py to avoid circular reference - changed functions calls to _add_reference_to_node_class and an unreachable branch that was not obvious before because the typing is incorrect here as input can be int as well (existing long standing problem, not something to be fixed in this PR as I didn't want to make this larger)
  • Add additional test coverage

@codecov
Copy link

codecov bot commented Mar 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.85%. Comparing base (172adda) to head (9db29d2).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   94.21%   94.85%   +0.64%     
==========================================
  Files           8       10       +2     
  Lines         380      389       +9     
  Branches       52       53       +1     
==========================================
+ Hits          358      369      +11     
+ Misses         11       10       -1     
+ Partials       11       10       -1     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 18, 2025

CodSpeed Performance Report

Merging #23 will improve performances by 15.42%

Comparing fix_set (9db29d2) with main (172adda)

Summary

⚡ 2 improvements

Benchmarks breakdown

Benchmark BASE HEAD Change
test_large_parse_yaml 4.2 s 3.7 s +15.42%
test_simple_parse_yaml 28.8 ms 24.7 ms +16.33%

@bdraco bdraco changed the title fix: debug setting dunder attrs fix: setting __config_file__ and __line__ Mar 18, 2025
@bdraco bdraco changed the title fix: setting __config_file__ and __line__ fix: setting __config_file__ and __line__ Mar 18, 2025
@bdraco bdraco marked this pull request as ready for review March 18, 2025 21:58
Copy link
Collaborator

@jpbede jpbede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM, thanks 👍

@jpbede jpbede merged commit fd8e31a into main Mar 19, 2025
14 checks passed
@jpbede jpbede deleted the fix_set branch March 19, 2025 06:50
@bdraco
Copy link
Member Author

bdraco commented Mar 19, 2025

thanks

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants