Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add module-level remove_unused_platforms option #97

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

garynthompson
Copy link

Hi @dflook,

I am a hobbyist maker who has been using MicroPytohn on embedded devices with limited storage.

The current libraries being used have module-level if statements to create cross-platform compatible interfaces.

This feature was added in my fork so that when minifying a file, we can specify a target platform and all non-target platform blocks are excluded.

I modelled both the annotations and debug features and decided that this only will work at a module level

  • I wanted to keep it as simple as necessary
  • I don't really want to encourage complex nested platform-specific code and instead want to encourage clean interfaces.

There is 100% code coverage on remove_unused_platform.py and I have confirmed that the Sphinx documentation compiles appropriately.

Please let me know if you are happy to include this upstream and if not, what changes are required.

Kind regards,

Gary

Signed-off-by: Gary Thompson <coding@garythompson.au>
@dflook
Copy link
Owner

dflook commented Jan 14, 2024

Hi @garynthompson, thanks for your effort on this PR. This is a good idea, and from a quick look the implementation looks good.

What libraries do you know of that use _PLATFORM in this way? It would be interesting to see how they determine it's value - I see you mentioned sys.uname or platform. A natural extension of this would be to automatically detect any such platform variable, but that would be a lot more effort.

Another thought I had is that this approach would work for any simple if condition, not just platform checks. I wonder if this could be generalised into a branch elimination transform that works with any simple condition. I don't know if it would have any use beyond platform checks though.

This looks good, i'll have a proper look soon.

@garynthompson
Copy link
Author

Hi @dflook,

To partially generalise the solution, I also added some options where any basic keyword could be changed. Technically, any variable name can be passed in.

The current implementation can be used for other use cases, however, the command line arguments would be unintuitive as the naming of the main argument and options is around platform removal. The actual transform just needs a key (--platform-test-key=_PLATFORM) and a value (--platform-preserve-value=linux).

A natural extension of this would be to automatically detect any such platform variable, but that would be a lot more effort.

It is an interesting idea, I haven't explored the different ways people use to explore platform tests. I vaguely recall seeing platform tests much more often about 12 years ago, but I could be getting confused with C. I was curious, but not curious enough to spend a lot of time on it this weekend.

Originally I was just going to check simple if KEY == VALUE: statements, but then decided that elif and else are useful as well. I kept it to Ast.EQ for explicitness (I often keep PEP20 in mind).

Here is an example of a python module for controlling some hardware that is cross-platform that inspired me to look into this:

https://github.com/CoreElectronics/CE-PiicoDev-Unified/blob/main/PiicoDev_Unified.py

I am writing my own projects using the PiicoDev hardware using both Raspberry PI 4 and Raspberry Pi Pico.

Happy to make any improvements you suggest, even if it seems as seemingly minor as variable naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants