Skip to content

Conversation

@justbuchanan
Copy link
Member

Implements #36.

This works by simply prepending __file__ = <path> at the top of the script if it's loaded from an on-disk path (not stdout).

I think it's debatable whether this is the most correct/pythonic approach for importing python modules (outlined in #36) as opposed to doing something like PYTHONPATH=path/to/other/modules cq-cli .... However, I am also using __file__ to find and load resources like stl/step files that exist in the project relative to the model file and PYTHONPATH doesn't really work there.

@justbuchanan justbuchanan requested a review from jmwright March 12, 2025 20:00
@justbuchanan
Copy link
Member Author

The tests related to parameter-passing are failing now because __file__ is showing up as a parameter and throwing off the order of the checks. For example, test_parameter_analysis is seeing this as the output parameter list: [{"type": "string", "name": "__file__", "initial": "/home/justin/src/github/cq-cli/tests/testdata/cube_params.py"}, {"type": "number", "name": "width", "initial": 1}, {"type": "string", "name": "tag_name", "initial": "cube"}, {"type": "boolean", "name": "centered", "initial": true}]

@justbuchanan
Copy link
Member Author

Ok, fixed all the tests except the freecad one, which was already failing.

@jmwright
Copy link
Member

Thanks @justbuchanan . I will fix the FreeCAD test separately, it does not need to delay this PR.

Are there any cases where injecting __file__ this way can cause issues with Python's built-in mechanism? Should the user have the option to disable this behavior?

@justbuchanan
Copy link
Member Author

Thanks for taking a look!

Are there any cases where injecting __file__ this way can cause issues with Python's built-in mechanism? Should the user have the option to disable this behavior?

Not that I'm aware of. In normal python execution, __file__ is set, so I don't think there's any harm in doing so here. I can put this behind a feature flag if you'd prefer, but I can't foresee any circumstances where the user would want to disable it.

@jmwright
Copy link
Member

One drawback I've thought of is that when users get a stack trace, the line number will be one off from their local copy of the code since a line was injected at the start of the script.

Copy link
Member

@jmwright jmwright left a comment

Choose a reason for hiding this comment

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

Looks good @justbuchanan . Let me know if you have any thoughts on shifting the stack trace line number down by one. I think we can merge as-is since cq-cli is more of a production tool than a development tool.

@jmwright jmwright merged commit e7a2d9b into CadQuery:main Mar 19, 2025
2 of 3 checks passed
@justbuchanan justbuchanan deleted the __file__ branch March 24, 2025 23:14
@justbuchanan
Copy link
Member Author

That's a fair point about the stack trace line numbers being off by 1. Looking at this again, a better solution would probably be to pass a value for __file__ into cqgi's CQModel.build() so that it gets added to the environment for exec() here: https://github.com/CadQuery/cadquery/blob/471ab6fcc79b6ce64b857d33d06202c030cf5459/cadquery/cqgi.py#L119. I'll take a look and see how difficult it would be to plumb that value through.

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