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

Update type.d.ts #1356

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Update type.d.ts #1356

merged 1 commit into from
Aug 19, 2024

Conversation

FishOrBear
Copy link
Contributor

No description provided.

Copy link
Contributor

@platypii platypii left a comment

Choose a reason for hiding this comment

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

We use Poly3.plane internally, but it's not really meant to be exposed to the user.

Question for @z3dev do we want:

  1. types to reflect the internal usage, so that we can typecheck more thoroughly within the project
  2. "hide" the internal representations from the user, so that users of the library don't assume it will be there

@z3dev
Copy link
Member

z3dev commented Jul 28, 2024

@FishOrBear Why add the plane to the class? Does this assist in some of you more difficult designs?

@FishOrBear
Copy link
Contributor Author

@z3dev I will access it in my program, but it is not always present, so I marked it as an optional parameter.

@z3dev z3dev self-requested a review August 17, 2024 04:50
@z3dev
Copy link
Member

z3dev commented Aug 17, 2024

@platypii what do you think? This is probably another example of users doing something that is unexpected. The changes are correct.

Copy link
Member

@z3dev z3dev left a comment

Choose a reason for hiding this comment

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

@FishOrBear these are interesting... but I'm a fan of being detailed. So, let's go with these changes.

please note that the API documents will not provide information on the optional planes.

Copy link
Contributor

@platypii platypii left a comment

Choose a reason for hiding this comment

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

I am in general a fan of having more type safety. I think having it as optional type means that users can't depend on it being there so we have optionality in the future. 👍

@z3dev z3dev merged commit 2a3337a into jscad:master Aug 19, 2024
2 checks passed
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