Skip to content

Narrow types and complete doc strings in shapes#2057

Closed
DavidMStraub wants to merge 3 commits into
CadQuery:masterfrom
DavidMStraub:shapes_type_narrowing
Closed

Narrow types and complete doc strings in shapes#2057
DavidMStraub wants to merge 3 commits into
CadQuery:masterfrom
DavidMStraub:shapes_type_narrowing

Conversation

@DavidMStraub

Copy link
Copy Markdown
Contributor

This PR narrows some of the type hints in the free function API (shapes module). Rather than formally accepting Shape, I added a union of the actually supported types. Compound is something that static type checking can do nothing about, so we just accept it. One unit test is added because omitting Compound from the type hints would have lead to a change in runtime behavior in some of the multidispatched functions which wouldn't have been caught by the existing test.

I also took the liberty to expand the doc strings of the modified functions, using the same Sphinx style for the parameter docs.

Claude Code used to prepare this PR.

Draft status as I expect black failures, will fix afterwards 😊

@adam-urbanczyk

Copy link
Copy Markdown
Member

I'm not sure that I agree with removing the Shape annotations. The design rule of the free func API was to support Shape inputs wherever possible. That is why there are _get_* methods

@DavidMStraub

Copy link
Copy Markdown
Contributor Author

I think I see your point - since the return type must by Shape in many cases, you want to be able to allow passing it as input as well, and instead raise at runtime. Correct?

What I don't like is the bare _get and _get_one with a string shape that returns a Shape - would be ok with me

  • replacing all their usages with the specifically typed _get_face etc.,
  • remove the type narrowing I added
  • but leave the doc string additions?

@adam-urbanczyk

Copy link
Copy Markdown
Member

_get and _get_faces have different semantics and _get supports multiple types. What are you trying to achieve by digging into these internals?

@DavidMStraub

Copy link
Copy Markdown
Contributor Author

What are you trying to achieve by digging into these internals?

I want to be able to write (& teach) code that has as useful type checks as possible.

The only reason those methods work with mypy is because of the plugin written specifically for that purpose. But many people (& companies) write code in VS Code where types are checked with Pylance, and they use pyright instead of mypy.

A red squiggle in the editor under a wrong input type is much more helpful than an exception at runtime (especially from the beginner's perspective).

But I see the problem now that _get_faces coerces while _get only validates, so you're right it's not possible to just swap them in.

I just pushed a change that (apart from removing my type narrowing) adds @overloads on _get and _get_one and should improve the situation without change in runtime behaviour.

What do you think?

This would make most of the mypy plugin redundant, actually.

@adam-urbanczyk

Copy link
Copy Markdown
Member

OK, I still don't get why you want to touch/use/teach those internals and why they would cause issues in user code. Regarding VSCode et al, it might be good to expose students to tools that are more open source oriented. Here is an experimental PR that gets rid of the plugin in a way that is somewhat acceptable (i.e. hardcodes max arity), I'm not sure yet if it is time well spent and the benefit justifies the cost: #2060

@DavidMStraub

Copy link
Copy Markdown
Contributor Author

My motivation was of course not to teach internals (heaven forbid) but to make public methods more reliably typed for static type checkers (= less surprises at runtime).

@adam-urbanczyk

Copy link
Copy Markdown
Member

Thanks, clear. Could confirm that #2060 results in the desired behavior with your toolchain?

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