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: SyntaxError #48

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rachmadaniHaryono
Copy link

@rachmadaniHaryono rachmadaniHaryono commented Feb 18, 2022

Proposed changes

Types of changes

Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@blueyed
Copy link
Collaborator

blueyed commented May 19, 2022

Thanks for this.
Haven't looked into it yet, but it seems like a rebase is required to trigger CI correctly!?

@rachmadaniHaryono
Copy link
Author

Unfortunately I don't have my computer for a month or 2 so I can't rebase it for now

@mattip
Copy link
Member

mattip commented May 20, 2022

Maybe closing/reopening will retrigger CI?

@mattip mattip closed this May 20, 2022
@mattip mattip reopened this May 20, 2022
@mattip
Copy link
Member

mattip commented May 20, 2022

CI on travis should probably be disabled, it costs "credits" to run on x86_64

@mattip
Copy link
Member

mattip commented May 20, 2022

CI is passing, where it can

pyrepl/keymaps.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

Apparently code formatting was also applied, likely using black?
I think only fixing errors might be better / easier to review, but am not opposed to have it as it is now.

pyrepl/keymaps.py Outdated Show resolved Hide resolved
@rachmadaniHaryono
Copy link
Author

Apparently code formatting was also applied, likely using black?

this one https://github.com/akaihola/darker

I think only fixing errors might be better / easier to review, but am not opposed to have it as it is now.

i dont know style used on this repo so i used darker formatter as default

if there is error due to code style i will remove it from pr and make the minimal change

@@ -77,8 +77,7 @@
(r"\<home>", "home"),
(r"\<f1>", "help"),
(r"\EOF", "end"), # the entries in the terminfo database for xterms
(r"\EOH", "home"), # seem to be wrong. this is a less than ideal
# workaround
(r"\EOH", "home"), # seem to be wrong. this is a less than ideal workaround
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now it appears with "home" only, but likely is also meant for "end". (just from what I think/see, no idea really).
I wanted to ensure that the comment's meaning/location/reference is kept (which was given before due to whitespace alignment), but now it is even less clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya I agree. It looks like the comment spans multiple lines and starts with "the entries in the terminfo database.." IMO it should be on a separate line, before the end & home lines.

Copy link
Author

Choose a reason for hiding this comment

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

0262f36 fix that

should there be new line between help and comment? if yes, i will create another commit for that

@millerjason
Copy link

@blueyed is this ready for merge?

@rachmadaniHaryono
Copy link
Author

i think it is ready

some suggestion already added to pr

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.

SyntaxErrors (Python 3)
5 participants