Add support for reading .pyc files

[dhruvkb]

Fixes

Fixes #563 by @dhruvkb

Description

This PR adds support for reading settings from .pyc files. It does so by using marshal.load instead of compile when the file extension is .pyc.

To verify that this works, it intentionally compiles an existing .py file into its .pyc before running the tests, checks that the contents of the file are still read into settings as expected, and then restores the .py file back.

[dhruvkb]

I've started working on the PR but I would need some help increasing the test coverage. Even with the added tests, it's currently at 96.34% (with a perplexing reverse line number range in the missing column).

---------- coverage: platform darwin, python 3.12.7-final-0 ----------
Name                      Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------
split_settings/tools.py      58      1     24      2    96%   127, 131->115
---------------------------------------------------------------------
TOTAL                        58      1     24      2    96%

[sobolevn]

Thank you! Looks very promising!

[dhruvkb]

The coverage is also a 100% now as some unnecessary (and unreachable) code paths were removed!

It's still in draft because point 1 of my issue remains to be addressed in a clean idiomatic way. I am thinking of defining a new wrapper (over tuple) similar to how _Optional and _Compiled wrap str to define a "chain" where each successive member is looked for till one is found and an error is raised if no member in the chain was found.

That way one could write the following snippet:

include(
    chain(
        "source_code.py",
        compiled("byte_code.pyc"),
    ),
)

[sobolevn]

Thanks!

[sobolevn]

I think that we need to test this more. Can you please create a new test with existing optional(compiled('file')) and compiled(optional('file'))?

What do you think: should we allow arbitary nesting? Or should we only use one way? Or maybe optional_compiled function?

[dhruvkb]

I think optional(compiled()) makes sense because it indicates that a compiled-file may optionally be present.

The opposite nesting compiled(optional()) seems to imply that a file may or may not be present but has to be compiled? That's not making sense to me.

It seems to me here that enabling the former nesting order covers the possible need and it becomes a question of having adequate documentation to discourage any accidental use of the latter nesting order.

Also compiled(optional(()) doesn't work (in the current implementation) because it loses the wrapping of _Optional once it is wrapped by _Compiled.

[sobolevn]

Please, don't forget to document that.

[dhruvkb]

@sobolevn I have been thinking more about the compiled() pattern that you suggested in this review comment and how that would work with wildcards.

Building on the example in the README.md, How would a wildcard like include('components/my_app/*.py*') (that can match both .py and .pyc) be supported?

[sobolevn]

I don't think that it is safe to include both source and compiled in wildcards. I would suggest to only support explicit compiled files.

[dhruvkb]

So would we filter the output of the glob function when it matches .pyc files?

[sobolevn]

Yes, we should raise an error when .pyc file matches a glob

[dhruvkb]

Okay, so there are a few issues with the docs that I will look into in a bit. I have to write some new docs about the additions here anyway.

But does the code approach taken here look good to you @sobolevn? I also added a bunch of test cases for all supported nesting of compiled, optional and one_of.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (654b1a6) to head (a09e84d).
Report is 41 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #565   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         3    +2     
  Lines           41       121   +80     
  Branches         7        23   +16     
=========================================
+ Hits            41       121   +80     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sobolevn]

I think that OneOf adds some other feature we didn't discuss ;)
It also feels like a very complex solution. Please, remove it.

I also think that Entry should not be used, because str worked just fine. Let's keep things as simple as possible :)

[dhruvkb]

I am going to have to bail on this PR.

• I stopped compiling the application so the original motivation for supporting .pyc files is my app is no longer applicable. Also I feel like this was a pretty niche request which makes it harder to justify such drastic changes to the code.
• OneOf would have been essential to support a .py file with a fallback to the compiled .pyc of the same file without having to make both entries optional (and lose the the guarantee that exactly one of .py or .pyc would be included).

I'm really sorry that I could not complete this, but deepest thanks for your time and your continuous advice on improving the code. It has been incredibly helpful.

Additionally, please feel free to close #563 if you too feel like it is adds complexity with little utility.

[sobolevn]

THANK YOU FOR YOUR WORK 👏
I had a huge fun reviewing this PR. Sad, that this won't be merged.