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

feat: python3.11 support #102

Merged
merged 6 commits into from
Apr 8, 2024

Conversation

ichintanjoshi
Copy link
Contributor

@ichintanjoshi ichintanjoshi commented Mar 28, 2024

Description

This PR contains changes for upgrading python 3.8 to python 3.11.
Changes include :-

  • Dependencies and version upgrades
  • Extra check for factorial function to get only integers
    • This is done because evaluator function in calc.py file will convert all numbers to float
    • And python 3.11 will not allow math.factorial to calculate float values and will raise an error ref
  • Removal of float numbers for factorial function tests (Same explanation as above)

Done as a part of following :-

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 28, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Mar 28, 2024

Thanks for the pull request, @ichintanjoshi! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@ichintanjoshi ichintanjoshi marked this pull request as ready for review April 3, 2024 11:36
@ichintanjoshi
Copy link
Contributor Author

@feanil all tests are green now

calc/calc.py Outdated
@@ -265,6 +265,11 @@ def eval_variable(x):
return all_variables[casify(x[0])]

def eval_function(x):
# This condition here is only for factorial function
if isinstance(x[1], numbers.Real):
Copy link
Contributor

Choose a reason for hiding this comment

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

@ichintanjoshi this felt a little late since by now all the other evaluation has happened, what do you think about this alternate proposal where we fix the eval functions upstream instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @feanil sure, that'd work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feanil what should be the next course of actions ?

As we discussed earlier, I agree with your changes and I think it's best to do it in that manner if they are appearing earlier in flow, instead of my approach.

How to proceed with those changes ? Should I change code in this branch ?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about you cherry-pick my commits onto your branch since you have more updates here and then we can land your PR and I'll close mine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @feanil

requirements/ci.txt Outdated Show resolved Hide resolved
ichintanjoshi and others added 3 commits April 3, 2024 19:46
Update evaluation functions to not coerce integers to floating point
numbers when not necessary.
@feanil feanil merged commit 95d2e58 into openedx:master Apr 8, 2024
6 checks passed
@openedx-webhooks
Copy link

@ichintanjoshi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@feanil feanil linked an issue Apr 8, 2024 that may be closed by this pull request
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Test openedx-calc on Python 3.11
3 participants