lukasschwab / stackit

StackOverflow queries from the command line
MIT License
314 stars 28 forks source link

Fix invalid attempt to use terminal colors on Windows machines #6

Closed schwartzmx closed 9 years ago

schwartzmx commented 9 years ago

Since it only took me a few seconds, here's the proposed solution PR I mentioned in the comment earlier here.

If you want to test it, and you are on a Mac for example. Replace if not os.name == 'nt' with if not os.name == 'posix' and see if you get any terminal colors.

You shouldn't. This will mirror that effect on Windows machines ('nt').

Concerns, questions?

Phil

Edit: @lukasschwab if you approve this, should also look at reverting README.md changes.

lukasschwab commented 9 years ago

This looks good!

@vickiniu did some testing last night, and the real reason we're not announcing Windows compatibility is that the stackit -e functionality depends on subprocess.Popen(), which was spitting some errors (if you want to test this and confirm, that would be very welcome! I noticed that your windows testing only tries stackit -s).

I think that should be a pretty simple process given your if not os.name == 'nt' conditions combined with this StackOverflow thread––it's definitely on our to do list.

We have two options:

  1. We can merge this now, assuming we can solve the subprocess.Popen() issue in a later commit. As I said, I think that will be a simple process, but I'm not sure. (There will also have to be a change to the way we fixed the problem of directories with spaces in them, which probably broke Windows compatibility).
  2. Don't merge this PR until we have fully implemented Windows support. This means the repo doesn't contain code to accommodate an OS we aren't supporting generally, until that general support has been established.

@schwartzmx what do you think?

schwartzmx commented 9 years ago

@lukasschwab It is up to you, I've tried some testing on -e in Windows PowerShell. Yes it won't work especially if you just run an executable script i.e. ./test.ps1, because of the way stackit is processing the string given as a executable. stackit is automatically assuming that the first word is a "command" such as python. So that could be one general problem. Will update this comment in a second with some findings from my Windows machine.

This is what I've gotten, which seems right to me... for the particular instance of running a python script.py command. I think the execute functionality needs some work in general, not only for Windows. It may not even be feasible because of the way commands are run. What if the user is calling something as simple as just: ls -xzfasga directory/ and want to know why that won't work? Do you want to try to handle those cases? Because right now stackit will crash because it assumes the second argument is a filename. So its a touchy subject, and I'm not sure if there is a concrete solution to solving all the abnormalities that can come along with this functionality.

Honestly it's a good idea, but the user can just as easily copy and paste the error from the command line into the -s "your error here", without having to leave the terminal, so I don't think I would ever use -e to be honest.

Let me know your thoughts.

Also @lukasschwab ping the other collaborators (@) and let's get their input, if they'd like to chime in.

Phil

lukasschwab commented 9 years ago

@schwartzmx I'm double-checking with Vicki to see what she was testing with when she determined that our Windows support was broken. There's a good chance, it seems, that she was testing with something besides a python script. If only I had a windows machine with which I could test this...

@vickiniu, @lanidelrey, and @eniasebiomo are all a bit busier than I am right now, which is why I'm handling the repo, but there's a good chance they'll all be more active soon. Y'all: this is a pretty big question about how we want to structure the project and the interface, would love to hear your input!

I'm going to merge in the pull request so stdout colors work on Windows (long timeframe and high complexity for full Windows support means it doesn't make sense to stall this minor functionality improvement), but we should keep the conversation going here for the time being.

schwartzmx commented 9 years ago

@lukasschwab I agree, this would be a good discussion to have. And I'm enjoying helping out the stackit crew, I think its a great utility to have. I'm also more than willing to help out in the long run, and perfect the utility. I mean I could just fork it, customize it to my needs, and be done but contributing is what open-source is about! If any of you need me, or want some input, just ping me @schwartzmx.

Cheers,

Phil

schwartzmx commented 9 years ago

@lukasschwab Also in the latest release, still getting attempts to use colors in windows powershell... And it also breaks now.

C:\Users\Fed-Phil> stackit -s "Test"
Searching for: Test ...

Tags:

←[94m1
Question: Why is processing a sorted array faster than an unsorted array?←[0m
Answer: ** You are the victim of [ branch prediction ](//en.wikipedia.org/wiki/Branch_predictor) fail. **

←[94m2
Question: Testing if something is hidden←[0m
Answer:

Since the question refers to a single element, this code might be more
suitable:

Traceback (most recent call last):
  File "C:\Users\Fed-Phil\Anaconda\Scripts\stackit-script.py", line 9, in <module>
    load_entry_point('stackit==0.1.3', 'console_scripts', 'stackit')()
  File "C:\Users\Fed-Phil\Anaconda\lib\site-packages\stackit\stackit_core.py", line 197, in main
    searchTerm(term, tags)
  File "C:\Users\Fed-Phil\Anaconda\lib\site-packages\stackit\stackit_core.py", line 100, in searchTerm
    printQuestion(question, count)
  File "C:\Users\Fed-Phil\Anaconda\lib\site-packages\stackit\stackit_core.py", line 116, in printQuestion
    print(pColor.BLUE + str(count) + "\n" + "Question: " + question.title + pColor.END + "\nAnswer: " + h.handle(soup.fi
nd("div", {"id": "answer-"+str(answerid)}).p.prettify()) + "\n")
  File "C:\Users\Fed-Phil\Anaconda\lib\encodings\cp437.py", line 12, in encode
    return codecs.charmap_encode(input,errors,encoding_map)
UnicodeEncodeError: 'charmap' codec can't encode character u'\u201c' in position 37: character maps to <undefined>
lukasschwab commented 9 years ago

@schwartzmx I have het to bundle these changes into a new release––waiting for PR 9 to merge, as it includes changes I'd say are major enough to warrant a new release (0.1.4). That should happen by the end of the day.

For now, a manual install (python setup.py install) should solve your issues on Windows.

By the way: once some core interface changes––such as paging and the elimination of scraping––are made I will release a version 2.0.0

schwartzmx commented 9 years ago

Got it. Just weird that it is breaking now when it wasn't before. And I agree, #9 looks good.

Edit: I'd like to work on the -e problem, at some point, that we were previously discussing. Just need some feedback.

WnP commented 9 years ago

@schwartzmx using click, as implemented in #9 should fix the color issue on windows -I said should as I don't have windows os to test it- let me know if you have some issue

and yes the -e option needs some refactoring too, it must test if arguments are directories or files

schwartzmx commented 9 years ago

Using os.path.isdir(), and os.path.isfile() can help. But there are some other abnormalities. Each argument would have to be checked it seems. I may hack at this later.

Phil

WnP commented 9 years ago

@schwartzmx but why not using the command as it is? on my laptop if I run:

>>> import subprocess
>>> proc = subprocess.Popen('ls Documents/false_dir', stderr=subprocess.PIPE, shell=True)
>>> print(proc.communicate()[1])
b'ls: cannot access Documents/false_dir: No such file or directory\n'

and

>>> proc = subprocess.Popen('ls Documents', stderr=subprocess.PIPE, shell=True)

works as expected... there's something odd I don't understand with the directory choice ^^