Skip to content

chore: Refactor MultiCode Expression ✨ #146

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

Merged
merged 3 commits into from
Nov 7, 2021
Merged

Conversation

yezz123
Copy link
Contributor

@yezz123 yezz123 commented Nov 5, 2021

I just refactor some functions, mostly was simple like:

  • Swap if & else branches.
  • Merge Else If Into Elif.
  • De Morgan Identity.

@yezz123 yezz123 requested a review from Cito as a code owner November 5, 2021 11:31
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

Hi @yezz123, thanks for your contribution.

I'll go through your changes as time permits. One remark upfront: I try to keep the code very close to the original GraphQL.js code, so that the task of keeping the code bases in sync stays managable. That's why I often keep certain constructs or order of code blocks even if the code could be simplified or optimized.

@yezz123
Copy link
Contributor Author

yezz123 commented Nov 5, 2021

Hi @Cito 👋

I got your idea about keeping the same one related to GraphQL.js code Yes, my changes are not something that touches the core, just a pythonic one related to refactoring some functions that need to function well and in a fast way.
I run the Local test all of the functions work smoothly ✨

@yezz123
Copy link
Contributor Author

yezz123 commented Nov 7, 2021

Hey @Cito I just fix all that you recommend to add, I agree with you in all of the changes look good and logical, we don't need to break the logic of it also we need to keep it in a pythonic way 🚀✨.

You could run all the workflows before the merge

@yezz123 yezz123 requested a review from Cito November 7, 2021 01:05
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

Thank you @yezz123

@yezz123
Copy link
Contributor Author

yezz123 commented Nov 7, 2021

Happy to contribute to one of the things that i use daily 🚀✨

@Cito Cito merged commit b1b9199 into graphql-python:main Nov 7, 2021
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