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

fix: Add PlanningPinToIndex to Python #1191

Merged

Conversation

Christopher-Chianelli
Copy link
Contributor

@Christopher-Chianelli Christopher-Chianelli commented Nov 8, 2024

JPyInterpreter

  • Allow annotations to specify a type to override the generated getter return type. When a type is specified, it goes through the generic PythonLikeObject to Java Object convertor.

Python

  • Add PlanningPinToIndex
  • Make PlanningPin use the new JPyInterpreter mechanism to remove code that converts a PlanningPin to Pinning filter

Build

  • Fix Python 3.12 environment name in tox.ini

@Christopher-Chianelli
Copy link
Contributor Author

Fixes #1190

@Christopher-Chianelli Christopher-Chianelli linked an issue Nov 8, 2024 that may be closed by this pull request
… of int

JPyInterpreter

- Allow annotations to specify a type to override the generated
  getter return type. When a type is specified, it goes through
  the generic PythonLikeObject to Java Object convertor.

Python

- Add PlanningPinToIndex
- Make PlanningPin use the new JPyInterpreter mechanism to remove
  code that converts a PlanningPin to Pinning filter

Java

- Make PlanningPinToIndex support Integer, since its Javadoc
  says null is a valid value
Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

The field should not support null values - there is no reason for why the field should have a reference type. If the Javadoc says it, let's fix the Javadoc.

@Christopher-Chianelli
Copy link
Contributor Author

Counterpoint:

  • @PlanningPin allows null/using Boolean (i.e. not allowing Integer will be inconsistent)
  • There is no performance benefit from not allowing Integer, since the field is automatically boxed by MemberAccessor
  • Users might have restrictions that prevent them from using primitive types.

@triceo
Copy link
Contributor

triceo commented Nov 10, 2024

  • @PlanningPin allows null/using Boolean (i.e. not allowing Integer will be inconsistent)

You could also say that we shouldn't repeat mistakes of the past.

  • There is no performance benefit from not allowing Integer, since the field is automatically boxed by MemberAccessor

I agree. Performance is irrelevant here. I simply do not see the point of allowing this field to have a third state (null).

  • Users might have restrictions that prevent them from using primitive types.

This particular type has not supported null since the start, and there were zero complaints. That tells me null is not necessary there. (Besides the fact that there is no practical use for putting null here - zero is the perfect default for this use case.)

We can add it if we ever see the need. But I am not a fan of adding options that nobody is calling for.

@Christopher-Chianelli Christopher-Chianelli changed the title fix: Add PlanningPinToIndex to Python, allow it to be Integer instead of int fix: Add PlanningPinToIndex to Python Nov 11, 2024
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
65.6% Coverage on New Code (required ≥ 70%)

See analysis details on SonarCloud

@triceo triceo modified the milestone: 1.16.0 Nov 11, 2024
@triceo triceo merged commit 0750c9d into TimefoldAI:main Nov 11, 2024
26 of 27 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.

PlanningPinToIndex Python
2 participants