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

Refactoring Code #335

Closed
wants to merge 9 commits into from
Closed

Refactoring Code #335

wants to merge 9 commits into from

Conversation

yezz123
Copy link

@yezz123 yezz123 commented Aug 7, 2021

Setup.py

  • Use with when opening file to ensure closure

Util.py

  • Remove unnecessary else after guard condition
  • Replace if statement with if expression
  • Remove unnecessary else after guard condition
  • Replace unneeded comprehension with generator
  • Inline variable that is immediately returned

Template.py

  • Replace if statement with if expression, Simplify if expression by using or
  • Replace list(), dict() or set() with comprehension
  • Swap if/else branches, Merge else clause's nested if statement into elif

Runtime.py

  • Remove unnecessary else after guard condition

PyParser.py

  • Replace yield inside for loop with yield from
  • Lift code into else after jump in control flow, Merge else clause's nested if statement into elif
  • Replace if statement with if expression

Pygen.py

  • Merge nested if conditions
  • Simplify conditional into return statement (removes comment)
  • Replace if statement with if expression, Simplify boolean if expression

Parsetree.py

  • Replace unneeded comprehension with generator
  • Merge else clause's nested if statement into elif
  • Replace unneeded comprehension with generator

Lookup.py

  • Swap if/else branches, Merge else clause's nested if statement into elif
  • Swap if/else branches, Remove unnecessary else after guard condition

Lexer.py

  • Replace if statement with if expression
  • Merge nested if conditions
  • Swap if/else branches, Remove unnecessary else after guard condition, Merge else clause's nested if statement into elif
  • Swap if/else branches, Remove unnecessary else after guard condition

Exceptions.py

  • Replace if statement with if expression, Use with when opening file to ensure closure

Ast.py

  • Replace multiple comparisons of same variable with in operator

Pygmentplugin.py

  • Replace if statement with if expression

Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

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

Thanks for the effort,

this breaks compat with py2 because of the yield from as mentioned. I'm not sure if we want to that. (maybe we should?)

cc @zzzeek

mako/exceptions.py Show resolved Hide resolved
mako/ext/babelplugin.py Show resolved Hide resolved
mako/lookup.py Show resolved Hide resolved
mako/pygen.py Show resolved Hide resolved
mako/util.py Show resolved Hide resolved
@CaselIT CaselIT requested a review from zzzeek September 3, 2021 16:24
@yezz123
Copy link
Author

yezz123 commented Sep 3, 2021

@CaselIT I Code this based on Python 3.X

@CaselIT
Copy link
Member

CaselIT commented Sep 3, 2021

@CaselIT I Code this based on Python 3.X

I just noticed that mako still supports 2.7.

It may be time to drop it, let's wait mike's opinion :)

@zzzeek
Copy link
Member

zzzeek commented Sep 4, 2021

it would have to be a new "minor" release number 1.2

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 7706cd6 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 7706cd6: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3064

@zzzeek
Copy link
Member

zzzeek commented Sep 10, 2021

these are good changes but i think there should first be a new mako 1.2 startup that is python 3 only, then these refactorings go in. anyone want to work on this?

@CaselIT
Copy link
Member

CaselIT commented Sep 13, 2021

will take a look

@CaselIT CaselIT mentioned this pull request Sep 13, 2021
8 tasks
@CaselIT
Copy link
Member

CaselIT commented Sep 13, 2021

will take a look

@zzzeek added https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3078

we need to run black, but probably after this is merged. the above patch will probably cause some conflicts with this one

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision d94a809 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Failed to create a gerrit review, git squash against branch 'main' failed

@CaselIT
Copy link
Member

CaselIT commented Oct 25, 2021

I'll try to fix the conflicts later

Change-Id: Icdadaf29df281378b6bcf2398acfd4520b8c59c5

# Conflicts:
#	mako/exceptions.py
#	mako/ext/pygmentplugin.py
#	mako/lexer.py
#	mako/parsetree.py
#	mako/pygen.py
#	mako/pyparser.py
#	mako/template.py
#	mako/util.py
#	setup.py
Change-Id: I5cc5d75ce180c509a466e9e2a6aef19a87a4b9e5
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision c6243b1 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset c6243b1 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3140

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

mike bayer (zzzeek) wrote:

a couple of changes including one type-related error that I think has to be restored. some bigger points:

  1. do we want to just go straight to python 3.7?

  2. do we have a ticket to fix the pkg_resources stuff?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3140

  • mako/ext/linguaplugin.py (line 27): if we use python 3.7 we can use nullcontext() here

    if fileobj is None:
    ctx = open(filename, "r")
    else:
    ctx = contextlib.nullcontext(fileobj)
    with ctx as file_:
    yield from self.process_file(file_)

https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

python 3.6 ends on december 23 this year

mako/ext/babelplugin.py Show resolved Hide resolved
mako/pygen.py Show resolved Hide resolved
mako/pygen.py Show resolved Hide resolved
mako/util.py Show resolved Hide resolved
@CaselIT
Copy link
Member

CaselIT commented Oct 25, 2021

updated in gerrit

@CaselIT
Copy link
Member

CaselIT commented Oct 25, 2021

do we have a ticket to fix the pkg_resources stuff?

I've added it to #340

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3140 has been merged. Congratulations! :)

@CaselIT
Copy link
Member

CaselIT commented Oct 25, 2021

thanks!

@yezz123
Copy link
Author

yezz123 commented Oct 25, 2021

@CaselIT is this PR merged in another way ?

@CaselIT
Copy link
Member

CaselIT commented Oct 25, 2021

yes, it was merged by with a commit here c47d172

@CaselIT
Copy link
Member

CaselIT commented Oct 25, 2021

I don't understand why it changed the author :\ I must have messed something up. sorry :(

@yezz123
Copy link
Author

yezz123 commented Oct 25, 2021

@CaselIT No problem, its a great thing to help one of the most thing i use daily 🚀

@sqla-tester
Copy link
Collaborator

Federico Caselli (CaselIT) wrote:

leftover for branch name change. this was already merged

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3064

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3064 has been abandoned. That means that at least for the moment I need to close this pull request. Sorry it didn't work out :(

@bourke bourke added this to Done in Mako prioritization Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants