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.

Sign up for GitHub

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

Jump to bottom

Update Python code for cairo-lang 0.8.1 release #230

Merged
merged 3 commits into from
Apr 7, 2022
Merged

Update Python code for cairo-lang 0.8.1 release #230

merged 3 commits into from
Apr 7, 2022

Conversation

kkovaacs
Copy link
Contributor

@kkovaacs kkovaacs commented Apr 7, 2022 •
edited
Loading

This PR fixes up some imports and the BlockInfo constructor so that our code works with cairo-lang==0.8.1.

It also has some extra commits that fix some pylint warnings or just make our code conform to PEP8 better (like importing at the module level).

@kkovaacs kkovaacs force-pushed the krisztian/cairo-lang-0.8.1-update branch from 1b1c2c5 to ff9141f Compare April 7, 2022 06:46
koivunej
koivunej suggested changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

I don't think it's a good idea for readability to move imports to the top regardless of what style guides say. Would rather use minimal scopes, and fix tests if the minimal scopes cause any problems.

@kkovaacs kkovaacs force-pushed the krisztian/cairo-lang-0.8.1-update branch from ff9141f to 23d5c62 Compare April 7, 2022 08:30
koivunej
koivunej approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Looking good!

@koivunej koivunej self-requested a review April 7, 2022 09:18
koivunej
koivunej approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Considered if the requirements-dev.txt is up to date, but it must be up to date enough since tests run. pip-compile has broken and needs to be updated sometime in the future at least.

@kkovaacs kkovaacs merged commit 9bf5f91 into main Apr 7, 2022
@kkovaacs kkovaacs deleted the krisztian/cairo-lang-0.8.1-update branch April 7, 2022 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mirko-von-Leipzig Mirko-von-Leipzig Mirko-von-Leipzig approved these changes

@koivunej koivunej koivunej approved these changes

@CHr15F0x CHr15F0x Awaiting requested review from CHr15F0x

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kkovaacs @koivunej @Mirko-von-Leipzig