Skip to content

199 remove autogen code#201

Open
rprospero wants to merge 6 commits intorefactor_24from
199_remove_autogen_code
Open

199 remove autogen code#201
rprospero wants to merge 6 commits intorefactor_24from
199_remove_autogen_code

Conversation

@rprospero
Copy link
Copy Markdown
Contributor

Adding units.py to .gitignore can be a problem because the problem them becomes deciding when units.py gets generated.

Instead, I decided to punt on the issue by eliminating the autogenerated files entirely. Instead, the code is dynamically added to the modules at runtime. This means that the units module can never be out of sync and developers can't make changes that will be overwritten.

A couple of bits of advice for reviewing:

  • It looks like I wrote a lot of code in init.py, but it's pretty much just the old code form _build_tables.py, with some minor chanes
    • Lines 167 and 463 changed from opening a file to putting everything in a StringIO buffer
    • In lines 171-172, I had to get the full path to the module code, since it's no longer running form code in the same directory
    • Lines 457-460 and 482-485 do the actual work of creating the modules
    • The use of exec is not a security concern here because it is not being called on untrusted strings. Any attacker attempting to produce a security hole here would need to be able to modify init.py or _units_base.py, which would already provide them all the privileges that they could have gained from exec.
  • The units.py and si.py files are still required to exist, since I need a module to add the dynamically generated functions onto.
  • I removed accessors.py entirely, since the accessor system was deprecated
  • I moved the unicode_superscript code into _units_base.py because that we the only place where the code was called and handling relative imports of dynamic imports was getting to be a pain.

The biggest disadvantage of this setup is that certain tools (e.g. MyPy) don't use the proper Python module loading system and do not see the members of the generated modules. I also fully concede that there might be a cleaner architecture through the use of getattr, but that would be a significant rewrite and I thought that a smaller, incremental PR would be better at this time.

Closes #199

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

Copy link
Copy Markdown

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

No quality gates enabled for this code.

See analysis details in CodeScene

Quality Gate Profile: Custom Configuration
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

Copy link
Copy Markdown
Contributor

@DrPaulSharp DrPaulSharp left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed review notes, I'm inclined to agree that this approach is an improvement over what we have and should be accepted on that basis, with us keeping an eye on possible improvements down the line.


import importlib
import os.path
import types
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line should be removed as the module is unused.

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