Pathlib open

[buhtz]

Here we are. I will give details about the improvements and arguments for them in direct code comments.

Intention

My intention was a "bug" I haven't reported in an Issue. I can't use orgpase in my unittests when using pyfakefs. The latter use "faked" Path objects that are not recognized by that line.

if isinstance(path, (str, Path)):

With this fix my own code using pyfakefs works well.

[buhtz]

What is the CI error with 3.6 about? I am not able to interpret the shell output.

[buhtz]

Good morning @karlicoss ,

it's your final decision because you are the maintainer. I am totally fine with this. I will update my PR as you wish; maybe to tonight.

But please give me the opportunity to answer your arguments. I don't assume that this will turn your opinions/decisions. But I think it is important because this is a public discussion. And please keep in mind that English isn't my nativ language. My knowledge about the exact meaning or weight of some vocabulary is partly limited. Therefore, there is not only the risk that some things could be misunderstood in terms of content but also understood as impolite. ☮️

I'm not sure I agree try/except are more resource efficient

I'm not a python core dev but from my understanding it is fact that it is more efficient. You run it 1.000 times. 1.000 time checking with an if costs more then 999 times just running and 1 time catching and exception.

it's going to be such a tiny amount, comparing to IO operations like .readlines, that it's just a waste of time to optimize try/catch

This wasn't about time optimization in the meaing of make orgparse running faster because it is slow now. It is just about saving energy (CPU cycles and RAM) in the context of sustainable software development (sometimes called Green Computing).

definitely ruins readability

This is for you not everyone else. It is a matter of taste and convention. The latter is if you are used to it the code becomes quit clear when reading it. But of course your taste counts here because you do most of the maintaining (reading code) stuff.

try/catch makes code less safe

It is not "pythonic" to keep all user mistakes or possible edge cases into account. That is why we have exceptions. In the end an exception is thrown, the users opens a ticket, etc. But of course we have to think about edge cases. Here I couldn't think about an edge case causing trouble and all your unittest where green.

e.g. what if something else throws AttributeError? E.g. if you make a typo in .rstrip. ...

You are absolutaly right about it. My mistake. The try block was to big. The rstrip() should be moved into the else part like this.

try:
    # This will raise an AttributeError if it's not a file-like object.
    all_lines = orgfile.readlines()
except AttributeError:
    # ...
else:
    all_lines = (line.rstrip('\n') for line in all_lines)

then it would hurt readability even more since you'll have to use else: block

Matter of taste.

it messes with mypy (since it can't infer the type information anymore)

That is the point. The type information doesn't matter anymore here. That is a feature of Python not a bug. ;) I also prefer type annotations but it took years to get that into Python because it isn't that important as some people think. It was always an advantage of Python that you don't have to think much about types. You don't have to control everything.

again making code less safe

This is a point I don't understand or don't see.

I appreciate thinking about performance

I don't think about that but energy and CO2.

[buhtz]

I removed the try..except blocks.

Using tox made all 83 tests green.

But mypy has something to say. Never used it before.

orgparse/__init__.py: note: In function "load":

orgparse/__init__.py:148: error: Incompatible types in assignment (expression has type "Generator[str, None, None]", variable has type "List[str]")  [assignment]
        all_lines = (line.rstrip('\n') for line in all_lines)

Not sure if and how to handle that.

[buhtz]

OK, I modified my PR as you asked. Can I add something more to get this PR going?

[buhtz]

Dear @karlicoss , can we merge that? That PR is blocking my next release.

[karlicoss]

Hi! Sorry -- I had an issue with github notification, only seen this now. I fixed the remaining mypy issue myself and merged -- will do a new release shortly