simplenote-vim / simplenote.py

Python API wrapper for the Simplenote web service
http://readthedocs.org/docs/simplenotepy/en/latest/api.html
MIT License
173 stars 23 forks source link

Fix typerror when offline #27

Open yuuki0xff opened 5 years ago

yuuki0xff commented 5 years ago

Simplenote.authenticate() and get_token() usually return a str. When offline, these methods return None. Some methods do not consider for this behavior. Therefore, None will be set to http header, and TypeError will occur when sending a request.

Related issue: cpbotha/nvpy#191

BROKEN CHANGE: authenticate() and get_token() no longer catch the IOError, BadStatusLine and part of HTTPError errors.

atomicules commented 5 years ago

Hi,

thanks. This is required in addition to #24 then and the fix that went in there?

I will try to look soon.

To be honest I don't think I've ever thought or cared about the offline situation. Until now...

yuuki0xff commented 5 years ago

24 and #27 fixes different bugs. #24 fixed a bug that occurred in the following situation:

  1. Get token when online.
  2. Get/update/delete note when offline. It will be raise HTTPException before v2.1.4.

27 (this pull request) fixes bug that occurred in the following situation:

  1. Get/update/delete note when offline and it have not token. It will be raise TypeError.

The error handling style of this library is golang style instead of try-catch style (standard of the python). However, it is not good to throw an exception under certain circumstances.

atomicules commented 5 years ago

Hey... sorry for the delay. Just to let you know I am looking at this and will get merged in soon.

atomicules commented 5 years ago

Ok, just one question...

we are now importing HTTPException, but not actually using it anywhere. Are these meant to be HTTPExceptions?


        try:
            token = self.get_token()
        except Exception as e:
            return e, -1
atomicules commented 5 years ago

Been trying to test this, but it's actually pretty fiddly for me since I mostly work remotely... so can't really disable the network there :smile:

Testing locally (also a pain as on wired desktop) with this branch OR master I get:

>>> import simplenote
>>> sn = simplenote.Simplenote("USER", "PASS")
>>> stuff = sn.get_note_list()
>>> stuff
(URLError(gaierror(8, 'hostname nor servname provided, or not known')), -1)
>>> stuff = sn.get_note("11554a046a65651984a2e262f730ab61")
>>> stuff
(URLError(gaierror(8, 'hostname nor servname provided, or not known')), -1)

I.e. I'm not really seeing any difference there. When performing get_token then:

master, offline

>>> thing = sn.get_token()
>>> thing
>>> thing is None
True

this pr, offline

>>> thing = sn.get_token()
Traceback (most recent call last):
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 1317, in do_open
    encode_chunked=req.has_header('Transfer-encoding'))
  File "/usr/pkg/lib/python3.7/http/client.py", line 1229, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/pkg/lib/python3.7/http/client.py", line 1275, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/pkg/lib/python3.7/http/client.py", line 1224, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/pkg/lib/python3.7/http/client.py", line 1016, in _send_output
    self.send(msg)
  File "/usr/pkg/lib/python3.7/http/client.py", line 956, in send
    self.connect()
  File "/usr/pkg/lib/python3.7/http/client.py", line 1384, in connect
    super().connect()
  File "/usr/pkg/lib/python3.7/http/client.py", line 928, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/pkg/lib/python3.7/socket.py", line 707, in create_connection
    for res in getaddrinfo(host, port, 0, SOCK_STREAM):
  File "/usr/pkg/lib/python3.7/socket.py", line 748, in getaddrinfo
    for res in _socket.getaddrinfo(host, port, family, type, proto, flags):
socket.gaierror: [Errno 8] hostname nor servname provided, or not known

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/atomicules/Code/github/simplenote-vim/simplenote.py/simplenote/simplenote.py", line 106, in get_token
    self.token = self.authenticate(self.username, self.password)
  File "/home/atomicules/Code/github/simplenote-vim/simplenote.py/simplenote/simplenote.py", line 83, in authenticate
    res = urllib2.urlopen(request).read()
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 525, in open
    response = self._open(req, data)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 543, in _open
    '_open', req)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 503, in _call_chain
    result = func(*args)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 1360, in https_open
    context=self._context, check_hostname=self._check_hostname)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 1319, in do_open
    raise URLError(err)
urllib.error.URLError: <urlopen error [Errno 8] hostname nor servname provided, or not known>

I am not really understanding the benefit of the PR.

However...

I am going to continue looking to see how to better handle errors when offline so we can get something we are both happy with.

yuuki0xff commented 5 years ago

I'm sorry for the late reply.

we are now importing HTTPException, but not actually using it anywhere. Are these meant to be HTTPExceptions?

  try:
       token = self.get_token()
   except Exception as e:
       return e, -1

No. get_token() will raise some exceptions as written in the documentation. I used Exception to handle those exceptions.

The purpose of editing authenticate() and get_token() was to tell application the root cause of the failure. Considering the impact to other apps that use this library, I should consider other approaches. Please give me a little time.

Been trying to test this, but it's actually pretty fiddly for me since I mostly work remotely... so can't really disable the network there

If you can use docker, you can easily create offline environment as follows. :-)

$ docker run --rm -it --net none -v /path/to/git/repository/of/simplenote.py/:/work python:3 bash
# cd /work
# pip install -e .
# python
>>> import simplenote
>>> sn = simplenote.Simplenote(user, pw)
>>> sn.get_note(key)
yuuki0xff commented 5 years ago

How about this?

https://github.com/yuuki0xff/simplenote.py/commit/aead4f53f5b529e0ad11b0438b5b11a20eef0916

atomicules commented 5 years ago

I'm sorry for the late reply.

No problem.

If you can use docker

I'm on NetBSD so unfortunately not an option. I can test on my desktop (also NetBSD) by taking interface up and down, it's just then I lose my remote session and other things, etc. It's just finding a convenient time, etc.

How about this?

Will take a look over the weekend. Thank you.

atomicules commented 5 years ago

Will take a look over the weekend

Didn't happen, sorry. I got busy again. Still meaning to look at this again. Hopefully soon.

atomicules commented 4 years ago

This is still on my todo list... it just moves to the next day everyday :disappointed: . Worst comes to the worst it's Christmas soon and I have a couple of weeks off and some so time free.

atomicules commented 4 years ago

After much delay I'm going to have to put this as a "won't fix/merge". That's based on it's current form and what I know so far.

That doesn't mean I won't consider another PR or an update to this PR , etc, but someone else is going to have to put in the work for testing and then convincing me it won't break anything else.

I've been trying to get to this for months and testing something that requires being offline is just too impractical for me:

I've pushed up the changes I was thinking about in case it's of use to anyone; but it's been awhile since I was looking at this and I can't recall the state of them, etc; Doesn't look like I did much at all unfortunately.