karlicoss / orgparse

Python module for reading Emacs org-mode files
https://orgparse.readthedocs.org
BSD 2-Clause "Simplified" License
371 stars 43 forks source link

Allow parsing for all duration formats #36

Closed j3soon closed 3 years ago

j3soon commented 3 years ago

Fix #35 by supporting all duration formats in org-duration.el, following the same parsing flow and naming conventions.

However, this fix contains a breaking change. The original implementation uses integer for duration by assuming that there will be no floating-point duration; while in contrast, orgmode treats the duration as float by default.

I'm wondering if such breaking change will cause some package versioning issue.

j3soon commented 3 years ago

Some additional notes on org-duration.el:

karlicoss commented 3 years ago

Thanks, this looks good! Especially for the comments & tests!

However, this fix contains a breaking change. The original implementation uses integer for duration by assuming that there will be no floating-point duration; while in contrast, orgmode treats the duration as float by default.

Yeah, I'd imagine not many people use it, but on the other hand, maybe best not to break it if possible. Considering having seconds in the effort is a pretty esoteric usecase? -- perhaps if the duration happens to be a whole number, just cast it to int? That way it would minimize the breaking potential. So, something like this

def parse_duration_to_minutes(duration: str) -> Union[float, int]:
    # doctests etc
    res = parse_duration_to_minutes_float(duration)
    return int(res) if res.is_integer() else res

What do you think?

j3soon commented 3 years ago

Yeah, I agree that seconds in duration are sparingly used. The newest commit is now backward compatible. (return int by default)

I found that other function's regex code is in global (I guess it may result in better performance). So, I also moved the regex part to global.

karlicoss commented 3 years ago

Thanks! Regarding regexes: they've been like that from the beginning (I'm not the original author). Probably makes little runtime difference (I'd imagine it's a trivial optimization for cpython), but might be nice to make it accessible on the top level in case someone wants to reuse a regex elsewhere. also mypy issue on 3.6, but I'll just merge and fix myself -- seems that it's note possible to add commits from maintainer if the fork is from organization https://github.com/isaacs/github/issues/1681

Also updated CI config so it runs for other's PRs, so next time something like this is easier to spot https://github.com/karlicoss/orgparse/pull/37

j3soon commented 3 years ago

Thanks for the quick merge! I have tested the version on pypi and confirmed it works.