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

Feature/color on win #57

Merged
merged 27 commits into from Jun 16, 2020
Merged

Feature/color on win #57

merged 27 commits into from Jun 16, 2020

Conversation

Cielquan
Copy link
Contributor

@Cielquan Cielquan commented May 28, 2020

Problem:

The problem is the missing support for colors on windows machines and is described in #55.

Changes:

  • I refactored the devtools.debug.Debug._env_bool classmethod as a function env_bool in devtools.prettier
    Reason: The method was not necessary to be a classmethod so I refactored it near env_true function which is used by the method.
  • I added tests for new function env_bool.
  • I introduced the highlight module which takes the roll to distinguish if highlighting is used. More info below
  • I added tests for the new highlight`module.
  • I changed Debug.__init__ and Debug.__call__ for the use of the new highlight.use_highlight function.

highlight module

On import the module will, when on a windows machine, try to activate windows ANSI support by setting the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag via SetConsoleMode. See here for more info: https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences. Depending on the outcome the color_active variable will be set.

The code which does the work was posted here: https://bugs.python.org/msg291732. Truth be told I only kinda get what the code does but I think the author knows what he does. I tested it on two Win10 machines and it works.

Next there is the use_highlight function. It took over the work to distinguish if
DebugOutput.str function called by Debug.__call__ should use highlighting. The function will first check if the Debug class instance got created with highlight=True. If not set it will check the PY_DEVTOOLS_HIGHLIGHT environment variable. If also not set it decides by checking if isatty(file_) and color_active variable are True. The latter part is new and the variable gets set on import (see above). The part before is just the old behavior.

fixes #55

EDIT: Info: My added tests succeed on windows but 21 others do not.

@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

Merging #57 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #57   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          364       374   +10     
  Branches        56        57    +1     
=========================================
+ Hits           364       374   +10     
Impacted Files Coverage Δ
devtools/debug.py 100.00% <100.00%> (ø)
devtools/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 082d8bb...9ec2d1b. Read the comment docs.

@Cielquan
Copy link
Contributor Author

Cielquan commented May 28, 2020

I don't quite get the coverage thing but will look into it.

EDIT: I messed with settings and fixed them with a force push.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Thanks for working on this.

Overall looks good (I'll have to assume most of this is okay since I don't have access to windows).

Two main things are:

  • I prefer single quotes over double quotes
  • I think we should enable tests for windows, even if we have to skip of xfail some tests

devtools/highlight.py Outdated Show resolved Hide resolved
devtools/prettier.py Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
@Cielquan Cielquan requested a review from samuelcolvin May 30, 2020
Copy link
Owner

@samuelcolvin samuelcolvin left a comment

This is looking great but unfortunately there are a number of conflicts.

If this is too annoying to fix, let me know and I'll fix as much of it as possible (I don't have windows so it'll involve a bit of guess work).

devtools/highlight.py Outdated Show resolved Hide resolved
devtools/highlight.py Outdated Show resolved Hide resolved
devtools/highlight.py Outdated Show resolved Hide resolved
tests/test_expr_render.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Owner

samuelcolvin commented May 31, 2020

hi @Cielquan I'm afraid you still have lots of conflicts - see the PR summary above - that's what I meant by move into utils, not just rename.

@Cielquan
Copy link
Contributor Author

Cielquan commented May 31, 2020

hi @Cielquan I'm afraid you still have lots of conflicts - see the PR summary above - that's what I meant by move into utils, not just rename.

Year I rebased the up-to-date master into the branch .. but I need to fix some things .. the util.py was not there on the older master.

@samuelcolvin
Copy link
Owner

samuelcolvin commented May 31, 2020

Year I rebased the up-to-date master into the branch .. but I need to fix some things .. the util.py was not there on the older master.

Yes sorry about that, my fault.

@Cielquan
Copy link
Contributor Author

Cielquan commented May 31, 2020

Year I rebased the up-to-date master into the branch .. but I need to fix some things .. the util.py was not there on the older master.

Yes sorry about that, my fault.

No problem. Thats normal to update master is it not?

I think it should now work. Lets see.

@samuelcolvin
Copy link
Owner

samuelcolvin commented May 31, 2020

yes, it's normal, but I try to always merge PRs in the order they were created so people don't have to fix conflicts caused by new PRs. However in this case, I was rushing to get some things fixed, thus merged newer PRs before this one.

@Cielquan
Copy link
Contributor Author

Cielquan commented May 31, 2020

Oh we got a circular import problem with the merge of highlight.py into utils.py.

I will refactor a bit to fix it.

@Cielquan
Copy link
Contributor Author

Cielquan commented May 31, 2020

Ok I refactored env_bool & env_true from prettier into utils.

I also moved tests into a new test_utils.py file.

@Cielquan
Copy link
Contributor Author

Cielquan commented May 31, 2020

@samuelcolvin
The test_print and test_print_subprocess tests are failing everywhere. I will look into it.

The windows pipline fails because it cannot find twine.
EDIT: The install cmd fails on win.

@Cielquan
Copy link
Contributor Author

Cielquan commented May 31, 2020

So I fixed linux/mac with removal of covdefaults package which I just introduced. Problem is that it seems it does not work correctly with coverage directly .. coverage html fails while pytest with pytest-cov works.
Problem is the fail_under config. Because of this I removed the package and changed the platform specific pragma to no cover for the windows parts. This should be fixed later when the windows tests are fixed.

The windows+ twine problem is another topic.

@Cielquan
Copy link
Contributor Author

Cielquan commented May 31, 2020

Sooooo .. finally done 🎆

The windows problem was pip install -U setuptools pip in make install. On windows pip is unable to update itself. You need to call pip with python to update it: python -m pip install -U setuptools pip

@Cielquan Cielquan requested a review from samuelcolvin May 31, 2020
@Cielquan
Copy link
Contributor Author

Cielquan commented Jun 6, 2020

@samuelcolvin Is it good to merge or do you need me to change something? 😺

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

sorry for the slow response, this looks great.

Just a few tiny comments and queries.

devtools/utils.py Show resolved Hide resolved
devtools/utils.py Outdated Show resolved Hide resolved
devtools/utils.py Outdated Show resolved Hide resolved
devtools/utils.py Outdated Show resolved Hide resolved
Cielquan and others added 3 commits Jun 11, 2020
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@Cielquan
Copy link
Contributor Author

Cielquan commented Jun 11, 2020

see above

@samuelcolvin samuelcolvin merged commit ff3bb9f into samuelcolvin:master Jun 16, 2020
16 checks passed
@samuelcolvin
Copy link
Owner

samuelcolvin commented Jun 16, 2020

Thanks so much for this, sorry it took so long to merge.

I'll make a new release soon.

@Cielquan
Copy link
Contributor Author

Cielquan commented Jul 29, 2020

Thanks so much for this, sorry it took so long to merge.

I'll make a new release soon.

When do you think you can release the new version? 😃

@samuelcolvin
Copy link
Owner

samuelcolvin commented Jul 29, 2020

sorry 🙏, been busy 🏃 .

Just need to fix #62, then I can release. Will try to do so today.

@samuelcolvin
Copy link
Owner

samuelcolvin commented Jul 29, 2020

done, sorry for the delay.

@Cielquan
Copy link
Contributor Author

Cielquan commented Jul 30, 2020

done, sorry for the delay.

No problem. Thanks a lot 😄

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.

print fails on windows
2 participants