Skip to content

Conversation

@t00sa
Copy link
Contributor

@t00sa t00sa commented Aug 15, 2025

This adds a feature which allows complex command line arguments to be written to a cache file and then re-used. Where an argument is specified in both the cache and on the current command line, the command line version takes priority.

Cache arguments themselves are not sticky, so if --save-cache and --use-cache are set on the command line, they will get written to the cache but ignored when the cache is next loaded. This prevents the cache from being inadvertently overwritten with new arguments.

This adds a feature which allows complex command line arguments to be
written to a cache file and then re-used.  Where an argument is
specified in both the cache and on the current command line, the
command line version takes priority.

Cache arguments themselves are not sticky, so if --save-cache and
--use-cache are set on the command line, they will get written to the
cache but ignored when the cache is next loaded.  This prevents the
cache from being inadvertently overwritten with new arguments.
@t00sa t00sa added this to the vn2.0 milestone Aug 15, 2025
@t00sa t00sa moved this to In Progress in Fab Development Tracker Aug 15, 2025
@t00sa t00sa linked an issue Aug 15, 2025 that may be closed by this pull request
t00sa added 2 commits August 15, 2025 13:45
A problem with TestArgumentCaching::test_disabled seems to be tripping
up the CI testing on github even though it works interactively.  Try
to fix this by removing the cache file before running the test.

Also take the opportunity to add some missing docstrings to the test
cases.
@t00sa t00sa marked this pull request as draft August 15, 2025 12:55
@t00sa
Copy link
Contributor Author

t00sa commented Aug 15, 2025

It looks like the CI problems may be a consequence of the earlier versions of python - and pyfakefs? - because the 3.11 tests have run through cleanly.

I've converted the PR into draft while I try to narrow down the problem.

This replaces the pyfakefs calls with tmp_path in the cache tests to
prevent failures with older versions of python from tripping up the CI
tests on github.
@t00sa
Copy link
Contributor Author

t00sa commented Aug 15, 2025

I was able to duplicate the problem interactively with python 3.9. Replacing the pyfakefs fixtures with pytest tmp_path has resolved the problem. Marking the PR as ready to go.

@t00sa t00sa marked this pull request as ready for review August 15, 2025 13:29
@t00sa t00sa added enhancement New feature or request Blocked Blocked by another issue labels Aug 15, 2025
@t00sa t00sa marked this pull request as draft August 15, 2025 13:30
@t00sa
Copy link
Contributor Author

t00sa commented Aug 15, 2025

This should probably go on after #476. It's going to conflict, so I'll need to update the branch once the other change is on.

@hiker
Copy link
Collaborator

hiker commented Aug 18, 2025

This new feature should also be added to the Fab basceclass, so the user experience across the various CLI is consistent.

So, it should go in after #414 (which after all is the only high priority item and a release blocker, ready to be reviewed and as such must have priority).

Some of the caching features were defined in the wrong places; the
argument group was added in FabArgumentParser and the post-parse_args
setup was entirely contained in the _parser_wrapper decorator.  This
prevents the class from being self-contained and makes it harder to
reuse.  This moves the methods into the _FabArgumentCache class.

This also creates a CachingArgumentParser which is a simple
combination of _FabArgumentCache and ArgumentParser.  This provides a
self-contained parser for use by infrastructure that does not use the
fab CUI.
Add the CachingArgumentParser class to FabBase as a drop-in
replacement for argparse.ArgumentParser.  Also add some tests
confirming the core caching functionality works as expected with
FabBase.
@t00sa
Copy link
Contributor Author

t00sa commented Sep 3, 2025

@hiker, that's a good suggestion. Now that the FabBase change has been merged, I've updated my branch, refactored the caching mixin slightly, and created a CachingArgumentParser class which extends the standard argparse.ArgumentParser class.

I've added this as a drop-in replacement for the parser in FabBase and added some tests which confirm the basic functionality. I'm interested in your opinion. All the work that touches FabBase is contained in commit cadabb0 so it can easily be removed from the PR if you don't think it is a good idea.

The _FabArgumentCache class is a mixin and only works when added to a
class which also inherits from argparse.ArgumentParser.  Ignore type
complaints related to missing attributes caused by references to
methods defined elsewhere.
@hiker
Copy link
Collaborator

hiker commented Sep 5, 2025

@hiker, that's a good suggestion. Now that the FabBase change has been merged, I've updated my branch, refactored the caching mixin slightly, and created a CachingArgumentParser class which extends the standard argparse.ArgumentParser class.

I've added this as a drop-in replacement for the parser in FabBase and added some tests which confirm the basic functionality. I'm interested in your opinion. All the work that touches FabBase is contained in commit cadabb0 so it can easily be removed from the PR if you don't think it is a good idea.

Looks nearly perfect to me: the only suggestion I have is to keep the type declaration to be argparse.ArgumentParser (since CachingArgumentParser is derived from this anyway). Reason for this is that a script derived from FabBase could provide its own version of an argument parser to FabBase, e.g. to provide a better description instead of the default one set in FabBase (not sure if the description can (officially, i.e. without accessing some undocumented features) be changed. Plus a user might want to do other customisations??). We should of course document that your class is recommended to be used even in this case, but as far as I can see there is no need to restrict/enforce this, and who knows what a user might decide to do.
If your class adds new features that changes the API (and is used in FabBase, then of course it must be declared as CachingArgumentParser). But as far as I can see, atm the API is still the unchanged.
Thanks!

@t00sa t00sa modified the milestones: vn2.0, vn2.1 Sep 8, 2025
@t00sa
Copy link
Contributor Author

t00sa commented Sep 17, 2025

Thanks @hiker, I'm glad you think this is useful. I'm going to pick this up again shortly, integrate your suggestions, and get the PR moving again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Blocked Blocked by another issue enhancement New feature or request

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Command line argument caching

2 participants