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

tests/test_formatting: use __file__ #85

Merged
merged 1 commit into from
May 6, 2019
Merged

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Apr 9, 2019

It could be used in more places there, but I think that is fine for now.

I've noticed this when copying the file to another name for a new test
being added there (to compare against master).

It could be used in more places there, but I think that is fine for now.

I've noticed this when copying the file to another name for a new test
being added there (to compare against master).
@Delgan
Copy link
Owner

Delgan commented Apr 9, 2019

Nice idea. However, this seems to fail on Windows. {file.path} is in lowercase, while __file__ preserves the path case.

c:\users\delgan\desktop\loguru\tests\test_formatting.py
C:\Users\delgan\Desktop\loguru\tests\test_formatting.py

@blueyed
Copy link
Contributor Author

blueyed commented Apr 9, 2019

Should {file.path} get fixed then maybe?
Otherwise feel free to just close this.

@Delgan
Copy link
Owner

Delgan commented Apr 11, 2019

Should {file.path} get fixed then maybe?

Yep, that's definitely a bug!
The behavior differs from the standard logging. I can't remember why I used normcase() (it's there since the first commit). Maybe did I mean normpath()?

About the proposition of replacing tests/test_formatting.py with __file__, I read that in the Python source code:

frozen modules don't always have __file__ set

Although I have trouble imagining Loguru's tests being executed this way, maybe it's best to avoid using __file__to avoid this bug by principle?

@Delgan Delgan merged commit 2af1e28 into Delgan:master May 6, 2019
@Delgan
Copy link
Owner

Delgan commented May 6, 2019

@blueyed Sorry for the delay before merging this, thanks for the improvement!

Also, I removed the normcase() for path in 786151c .

@blueyed blueyed deleted the minor branch May 7, 2019 06:30
This pull request was closed.
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.

2 participants