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

add header and color options #12

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

mvrozanti
Copy link

No description provided.

@mvrozanti mvrozanti changed the title add header option add header and color options Apr 27, 2020
@daturkel
Copy link

This would be a great feature. I'd love to help get it merged but I can't tell what is failing in the CI. Any help @dahlia ?

@mvrozanti
Copy link
Author

I see a message about python 2 usage on the beggining of the console output. Maybe this has something to do with it?

@daturkel
Copy link

I'm guessing the python2 tests are failing with the f-string syntax. Try replacing:
f'{k}:{v}'
with
str(k) + ":" + str(v)

@mvrozanti
Copy link
Author

I just replaced this part but the build still fails. That's weird

@daturkel
Copy link

this one looks like a different error. i defer to @dahlia who probably has a better understanding of the CI setup. but the syntax error from before is gone and now we just have:

lint run-test: commands[0] | flake8 iterfzf.py setup.py
iterfzf.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: 'iterfzf.py'
ERROR: InvocationError for command 'C:\projects\iterfzf\.tox\lint\Scripts\flake8.EXE' iterfzf.py setup.py (exited with code 1)

@dahlia
Copy link
Owner

dahlia commented Jun 27, 2020

@mvrozanti Sorry for my late response. As @daturkel explained, it was an error that has existed before changes you made, and I finally fixed the error in the master. The error would be gone if you rebase your commits on the current master.

@mvrozanti
Copy link
Author

mvrozanti commented Jun 27, 2020

Still failing 😕 .

I actually just tried pulling from upstream. Do I really need to rebase?

Copy link
Owner

@dahlia dahlia left a comment

Choose a reason for hiding this comment

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

  • Please make its style consistent with the existing code. You can run the lint to check style: tox -e lint (you should install tox first: pip install tox). FYI the CI build is failing due to style inconsistencies:

    iterfzf\__init__.py:55:70: E231 missing whitespace after ','
    iterfzf\__init__.py:55:80: E501 line too long (91 > 79 characters)
    
  • Please write changelogs in the README.md file.

@@ -29,7 +29,7 @@ def iterfzf(
# Search mode:
extended=True, exact=False, case_sensitive=None,
# Interface:
multi=False, mouse=True, print_query=False,
multi=False, mouse=True, print_query=False, header=None, color=None,
Copy link
Owner

Choose a reason for hiding this comment

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

Could you move header parameter to # Layout: section? It looks more appropriate there.

@@ -1,4 +1,4 @@
from typing import AnyStr, Iterable, Optional
from typing import Dict, AnyStr, Iterable, Optional
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
from typing import Dict, AnyStr, Iterable, Optional
from typing import AnyStr, Dict, Iterable, Optional

Imports should be listed in lexicographical order.

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.

3 participants