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

Item#references: support keywords with UTF8 symbols #485

Merged
merged 3 commits into from
Aug 5, 2020
Merged

Item#references: support keywords with UTF8 symbols #485

merged 3 commits into from
Aug 5, 2020

Conversation

stanislaw
Copy link
Member

@stanislaw stanislaw commented Aug 4, 2020

This is a starting point for fixing #484.

For now, a test case is added with the français keyword as reported in the issue. In the test case, the reference is found without a problem on macOS. Let's see if the test fails on the other systems supported by CI.

Later update: the test indeed was failing on Windows. The solution suggested by @Korijn was to replace the usage of pyficache with linecache.

@stanislaw
Copy link
Member Author

@jacebrowning I need your advice on this. I noticed this while working on the multiple references feature one year ago: all tests work with the shared files/ folder so that adding any new test fixture (in this case doorstop/core/tests/files/REQ-UTF8.yml) requires modifications in the unrelated tests. This was extremely painful and is still relevant to date.

Would it make sense to create another folder for the test fixtures where they would be maintained "one folder per test case" such as test_fixtures/001-multiple-references-utf8-keyword so that no conflicts would be possible anymore?

If I am good to go, I would do this change right away.

@stanislaw
Copy link
Member Author

stanislaw commented Aug 4, 2020

@Korijn, pyficache -> linecache doesn't seem to be a trivial change.

 File "/.../doorstop/doorstop/core/reference_finder.py", line 48, in find_ref
    lines = linecache.getlines(path)
  File "/Users/Stanislaw/.pyenv/versions/3.6.0/lib/python3.6/linecache.py", line 47, in getlines
    return updatecache(filename, module_globals)
  File "/Users/Stanislaw/.pyenv/versions/3.6.0/lib/python3.6/linecache.py", line 136, in updatecache
    with tokenize.open(fullname) as fp:
  File "/Users/Stanislaw/.pyenv/versions/3.6.0/lib/python3.6/tokenize.py", line 454, in open
    encoding, lines = detect_encoding(buffer.readline)
  File "/Users/Stanislaw/.pyenv/versions/3.6.0/lib/python3.6/tokenize.py", line 431, in detect_encoding
    encoding = find_cookie(first)
  File "/Users/Stanislaw/.pyenv/versions/3.6.0/lib/python3.6/tokenize.py", line 395, in find_cookie
    raise SyntaxError(msg)
Exception: invalid or missing encoding declaration for '/.../doorstop/reqs/assets/logo-black-white.png'

I might take a closer look later or feel free to check my branch and use it as a base.

@jacebrowning
Copy link
Member

Would it make sense to create another folder for the test fixtures where they would be maintained "one folder per test case"?

@stanislaw Yeah, that makes sense to me!

@Korijn
Copy link

Korijn commented Aug 4, 2020

I see that it's failing on a .PNG file, I don't think that needs to be supported, right?

It looks like pyficache would return None for unreadable files instead of raising SyntaxError. I think we can just try/except that?

I can have a look and help fix this later.

Thanks for picking this up btw!

@Korijn
Copy link

Korijn commented Aug 5, 2020

By the way, I checked, pyficache indeed returns None for a .png file.

@stanislaw stanislaw changed the title WIP: Item#references: support keywords with UTF8 symbols Item#references: support keywords with UTF8 symbols Aug 5, 2020
@stanislaw
Copy link
Member Author

The last step is to remove pyficache from Poetry's dependency list. I will look into this later this evening because I am having some macOS dev toolchain-related complication warnings as I am running poetry.

@stanislaw
Copy link
Member Author

Would it make sense to create another folder for the test fixtures where they would be maintained "one folder per test case"?

@stanislaw Yeah, that makes sense to me!

@jacebrowning Thanks! The change is in. This should simplify work with these test cases in the future!

@Korijn
Copy link

Korijn commented Aug 5, 2020

Awesome work! Thanks :)

Copy link
Member

@jacebrowning jacebrowning left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@jacebrowning jacebrowning merged commit dc06db1 into doorstop-dev:develop Aug 5, 2020
@jacebrowning
Copy link
Member

I've uploaded this change here: https://pypi.org/project/doorstop/2.2b1/

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.

None yet

4 participants