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

Handle non-uniform behaviour of __builtins__ #581

Merged

Conversation

SimShady
Copy link
Contributor

@SimShady SimShady commented Aug 20, 2024

Modifications:

According to https://docs.python.org/3.9/reference/executionmodel.html:

CPython implementation detail: Users should not touch builtins; it is strictly an implementation detail. Users wanting to override values in the builtins namespace should import the builtins module and modify its attributes appropriately.

The builtins namespace associated with the execution of a code block is actually found by looking up the name builtins in its global namespace; this should be a dictionary or a module (in the latter case the module’s dictionary is used). By default, when in the main module, builtins is the built-in module builtins; when in any other module, builtins is an alias for the dictionary of the builtins module itself.


This fix is needed to package scikit-network in nixpkgs (or at least to be able to remove the workaround)

Impacted submodules:

None

This pull request:

(your PR must match these criteria before review)

  • targets the develop branch
  • does not decrease code coverage
  • passes the tests
  • is documented and the documentation build passes (does this need documentation?)
  • has PEP8-compliant code and explicit variable naming
  • has a tutorial (facultative but recommended) (is this applicable for this PR?)

@SimShady SimShady force-pushed the properly-set-builtins-if-dict branch from f3746b5 to fb905b7 Compare August 22, 2024 15:45
@SimShady SimShady force-pushed the properly-set-builtins-if-dict branch from fb905b7 to 5467ca9 Compare August 22, 2024 16:42
@tbonald tbonald requested a review from mclegrand August 26, 2024 09:39
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.62%. Comparing base (2cb2798) to head (5467ca9).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #581   +/-   ##
========================================
  Coverage    97.62%   97.62%           
========================================
  Files          182      182           
  Lines         8184     8184           
========================================
  Hits          7990     7990           
  Misses         194      194           

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

Copy link
Contributor

@mclegrand mclegrand left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@tbonald tbonald merged commit f7507f3 into sknetwork-team:develop Aug 26, 2024
10 checks passed
@tbonald
Copy link
Contributor

tbonald commented Aug 26, 2024

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.

3 participants