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

Separate ConfigSpec parser (Address #1636) #1639

Merged
merged 2 commits into from
Jul 29, 2023

Conversation

Julian-O
Copy link
Contributor

Separate ConfigParser code from Buildozer.

  • Use subclass, rather than monkey-patching.
  • Single implementation of list-handling code.
  • Convenience functions to minimise impact of existing code (but instantly deprecated).
  • Treat defaults consistently
  • Apply the env overrides automatically (client doesn't need to trigger).
  • Apply the env overrides once (read-time) rather than on every get.
  • Avoid re-using term "raw" which has pre-existing definition in ConfigSpec.
    • Update android.py client to match,
  • Unit tests.

Add comments to start and end of default.spec to explain the syntax (as discussed in #1636)

@misl6
Copy link
Member

misl6 commented Jul 22, 2023

Hi @Julian-O ,
Sorry to bother you, but can you please rebase on top of the latest master so we can have a clean CI build? 😀

Separate ConfigParser code from Buildozer.

- Use subclass, rather than monkey-patching.
- Single implementation of list-handling code.
- Convenience functions to minimise impact of existing code (but instantly deprecated).
- Treat defaults consistently
- Apply the env overrides automatically (client doesn't need to trigger).
- Apply the env overrides once (read-time) rather than on every get.
- Avoid re-using term "raw" which has pre-existing definition in ConfigSpec.
   - Update android.py client to match,
- Unit tests.

Add comments to start and end of default.spec to explain the syntax (as discussed in kivy#1636)
@Julian-O
Copy link
Contributor Author

@misl6: Delighted to. I am feeling more comfortable now the main tests are clearly passing. (At the time of writing, the slower ones are still running.)

@Julian-O
Copy link
Contributor Author

Tests are all green! Yay

@misl6
Copy link
Member

misl6 commented Jul 22, 2023

@misl6: Delighted to. I am feeling more comfortable now the main tests are clearly passing. (At the time of writing, the slower ones are still running.)

Seems that the commit has been added, instead of a rebase, so it shows up in the changes. You can rebase instead?

@Julian-O Julian-O force-pushed the factoroutconfigspec2 branch from 737b0de to 8c8ea02 Compare July 23, 2023 01:02
@Julian-O
Copy link
Contributor Author

@misl6: I think that's fixed. Still learning a new git front-end.

@misl6
Copy link
Member

misl6 commented Jul 23, 2023

What are your thoughts about _merge_config_profile ?

@Julian-O
Copy link
Contributor Author

Julian-O commented Jul 23, 2023

My thoughts? A little deflated at the moment. :-)

The first time I looked at it, I noticed it was not being monkey patched like the other "methods". I was mislead by its name into thinking it was merging two configs, and I thought it had build-specific logic in there, so I wanted to keep it in Buildozer.

I see now it is more general than that, and belongs in SpecParser...

... but it should be called implicitly. The clients shouldn't need to know when to call it. I think the profile name should be passed to the SpecParser's constructor so it can handle this...

But that isn't available until after the command line arguments are parsed. This is entirely consistent with my longer-term view that Buildozer should be split into BuildozerCommand (which is responsible for handles all the parsing of the options, and help) and Builder (which is responsible for knowing how to build an app). The Builder should be constructed by BuildozerCommand and give it all the parameters it needs in the constructor - including the Profile name, so it can construct the SpecParser.

But that is detouring me more and more from my primary goal. I want to get to the "move BuildOps into its own class, and make them platform independent". I am holding back on that PR while I set the groundwork.

Let me think on this. I will try to find a compromise - like moving _merge_config_profile into SpecParser, but requiring the clients to explicitly call it, and leave fixing that to the future.

Move it from Buildozer.
Rename it to be more meaningful.
Add support for whitespace trimming.
Update documentation in default.spec.
Update tests.
@Julian-O Julian-O force-pushed the factoroutconfigspec2 branch from cb0b147 to 1f703f7 Compare July 24, 2023 06:38
@Julian-O
Copy link
Contributor Author

How about that?

@misl6
Copy link
Member

misl6 commented Jul 29, 2023

How about that?

Love that last commit.

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

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.

2 participants