WIP: Futures #153
WIP: Futures #153
Conversation
| @@ -27,6 +34,9 @@ | |||
| get_ipython = lambda: None # pragma: no cover | |||
|
|
|||
| import lazy_import | |||
| import nbclient | |||
There was a problem hiding this comment.
nbclient and nbparametrise are missing in the dev-environment. I had to install them manually.
| @@ -11,6 +11,17 @@ services: | |||
| - "3306:3306" | |||
| volumes: | |||
| - ./compose/config/mysql/create_tables.sql:/docker-entrypoint-initdb.d/create_tables.sql:ro | |||
| db2: | |||
There was a problem hiding this comment.
Why did you add a second database? I don't see any usage
| #python_path=/home/wilfred/workspace/climdex-calc | ||
| #module=climdex_calc | ||
| [plugin:climdexcalc] | ||
| python_path=/home/wilfred/workspace/climdex-calc |
There was a problem hiding this comment.
local path which I don't have. I changed the path to locally checked out climdexcalc-plugin but the module name also does not correspond to the one online (climdex_calc vs climdexcalc)
| @@ -0,0 +1,148 @@ | |||
| { | |||
There was a problem hiding this comment.
This took me a while: Those are the templates to create content from the futures, aren't they? Please put a separate README in there.
I tried multiple times to execute those notebook like an idiot..
| @@ -309,7 +308,7 @@ def databrowser( | |||
| multiversion: bool, default: False | |||
| Select all versions and not just the latest version (default). | |||
| batch_size: int, default: 5000 | |||
| Size of the search query. | |||
| Size of the search querey. | |||
| futures.Futures.register_future_from_history_id(args.from_id) | ||
| else: | ||
| if not args.future_definition: | ||
| self.parser.error("You have to specify a future template.") |
There was a problem hiding this comment.
The error message says future template, but the arg in the ArgumentParser is called future_definition. Please stick to one word. I was not sure if both are the same.
| else: | ||
| if not args.future_definition: | ||
| self.parser.error("You have to specify a future template.") | ||
| raise SystemExit |
There was a problem hiding this comment.
This line is unreachable as self.parser.error throws an Exception by itself
|
|
||
| @classmethod | ||
| @handled_exception | ||
| def register_future_from_history_id( |
There was a problem hiding this comment.
register_future_from_history_id does not consume any kind of parameter except for --from-id, right? Can we make the argument parser more strict so that it does not allow any arguments except for --from-id in this case? I find it confusing that the parameters are passed but don't do anything
There was a problem hiding this comment.
If that is deliberate than I would have at least an information in the logs that the parameter gets ignored because of $random_reason
|
|
||
| self._add_facets_to_parser( | ||
| "Set <project> information", | ||
| suffix="if the can't be found in the metadata", |
There was a problem hiding this comment.
Besides, suffix does not create a whitespace (because the default is a dot I guess), therefore it concatenates with the description in the passed argument before it. This does not look good when typing --help
| if not args.future_definition: | ||
| self.parser.error("You have to specify a future template.") | ||
| raise SystemExit | ||
| futures.Futures.register_future_from_template( |
There was a problem hiding this comment.
time, cmor_table, model are not passed to register_future_from_template, but are valid parameters.
There was a problem hiding this comment.
If time should be possible: Can we provide examples of valid time-Values? This is not obvious from the help.
This is a work in progress PR.
Test are not working, I still need to add unit (functional tests).
But that shouldn't prevent you from already starting to review it, mainly because this is a big one. It would be good if you could have a more in depth look at this. That is the database table structure, solr config and especially the code it self.
Very much appreciated.