Closed andersgb1 closed 6 months ago
@diezo could this be of interest?
And thanks for an awesome library btw.!
@andersgb1 thanks for your contribution. i'll merge this pr after running some tests.
@diezo nice! I just made a small extra commit, since the PR was originally returning /p/ links for reels. Although they are valid, it's changed to /reel/ links now so it aligns with the platform
Hello @andersgb1 I notice the some times you are returning or yielding None before raise an exception:
yield None
raise NetworkError("HTTP Response is not a valid JSON.")
There is any reason for that?
My concern is that can obfuscate some issues, for example:
for reel in host.reels():
# here we will have a "NullPointerException"
# AttributeError: 'NoneType' object has no attribute 'title'
print(reel.title)
# instead of having the network error
Hello @andersgb1 I notice the some times you are returning or yielding None before raise an exception:
yield None raise NetworkError("HTTP Response is not a valid JSON.")
There is any reason for that?
My concern is that can obfuscate some issues, for example:
for reel in host.reels(): # here we will have a "NullPointerException" # AttributeError: 'NoneType' object has no attribute 'title' print(reel.title) # instead of having the network error
It's basically a copy-paste of how it's done in posts(): https://github.com/diezo/Ensta/blob/master/ensta/Guest.py#L284-L286, so what you ask is not specific to this PR, I believe.
I was not here when the Guest.py code was added, I don't think that it had a PR, I would have pointed the same thing
So I understood that there is no specific reason for the yield None
I'll not merge or accept this PR, but other maintainers, like @diezo, can do it
I was not here when the Guest.py code was added, I don't think that it had a PR, I would have pointed the same thing
So I understood that there is no specific reason for the
yield None
I'll not merge or accept this PR, but other maintainers, like @diezo, can do it
Makes sense. Maybe an issue on a refactoring would be appropriate?
@andersgb1 @ettoreleandrotognoli Thanks for pointing out the potential issue. For now, I'll merge this PR and fix it in a later commit.
Addresses #89