Automatix_cmd Merge #66
Conversation
Experimental features should be deactivated on default, even though it's a cool feature ;)
To avoid over long and changing lines.
Also python requirement increased to >=3.10
Add RUNNING_INSIDE_AUTOMATIX environment variable
Handing over all the globals of the main programm seems unnecessary and dangerous.
Builtins seem to be included anyway.
a_vars to avoid shadowing vars() Also adding tests for assignment.
This is to have a better separation between the PERSISTENT_VARS the user sets and the programmaticly set variables, like a_vars, PVARS, AbortException, ... Fixes also a bug, where assignments in python commands did not work, because the a_vars had been overridden by PERSISTENT_VARS.
Refactor python vars and add Python debug shell
Some screen versions do not have 'GNU' or 'FAU' in their version output. We do not know which version it might be, but we can have a try.
Since version 5.0.0 of GNU Screen the term "GNU" is no longer part of the version output. This makes it difficult to identify the GNU version safely. Therefore we asume the correct version if we do not have clear hints for the false (FAU) version.
Fix screen version check
Add example for file content assignment with "FILE_" syntax
jpaul-seibert
requested review from
dracador,
phosei,
fkusei and
sschmachtel
|
Also fixes #64 |
|
@sschmachtel and others: if it helps, have a look at https://github.com/vanadinit/automatix_cmd/pulls?q=is%3Apr+ |
README.md
Outdated
| @@ -19,7 +19,7 @@ There are different modes for **automatix** to work. Without any | |||
| commandline step whether to execute, skip or abort. | |||
| Forced mode (**-f**) will also proceed if errors occur. | |||
|
|
|||
| **automatix** is originally designed for internal //SEIBERT/MEDIA use. | |||
| **automatix** was originally designed for internal seibert// use. | |||
There was a problem hiding this comment.
I am not sure about the "correct" naming here. Isn't seibert// the brand name? Would you write Seibert Group or seibert// group? Or just inserting a link to the homepage?
There was a problem hiding this comment.
According to the branding guidelines, we never put "seibert//" in text, only ever in the logo. In writing, we should use "Seibert" or "Seibert Group".
|
|
||
| # Bundlewrap support, bundlewrap has to be installed (default: false) | ||
| bundlewrap: true | ||
|
|
||
| # Teamvault / Secret support, bundlewrap-teamvault has to be installed (default: false) | ||
| teamvault: true | ||
|
|
||
| # Activate progress bar, python_progress_bar has to be installed (default: false) | ||
| progress_bar: true |
There was a problem hiding this comment.
Can we have this switch automatically if the package is installed?
There was a problem hiding this comment.
It is no longer a extra package. All the functionality is included in automatix.progress_bar.
| # answers 'a', 'c' and 'R' are handled by _ask_user, 'p' means just pass | ||
| err_answer = self._ask_user( | ||
| question='[CF] What should I do?', | ||
| allowed_options=['p', 'T', 'D', 'v', 'r', 'R', 'a'], |
There was a problem hiding this comment.
Reading this a couple of times, i think we should maybe have a NamedTuple for these Options instead of handing strings around.
There was a problem hiding this comment.
I am not sure about your intention here and whether NamedTuple improve the code here, but I catch the feedback, that it is difficult to read and understand.
I haven't been using NamedTuples so far and from what I read, the main advantage is the object notation.
Maybe this should be refactored anyway. Maybe also a dataclass might do the thing? Maybe we should have a long descriptive option naming instead of characters?
What was your main point? Not be able to easily understand, what the single chars stand for?
There was a problem hiding this comment.
I refactored this in #71
Please have a look at it and give feedback :)
| continue | ||
|
|
||
| print(red(f'Warning: environment variable "AUTOMATIX_{c_key.upper()}" ignored: wrong value type!')) | ||
| sleep(2) |
There was a problem hiding this comment.
Is it worth exit()ing here if errors occur?
There was a problem hiding this comment.
You mean whenever an environment variable is set but is not intended to be set, we should exit?
I think this is actually only possible, if you try to use AUTOMATIX_CONSTANTS. I do not think, it makes sense to set this environment variable.
You might also run into this, if you fill your configuration file with values, which are not intended, e.g. setting encoding to a list or dict.
It might be considerable to "input check" the values to avoid dangerous behaviour, but I think this should be part of a seperate PR, if desired.
| logfile_dir = f'{CONFIG.get("logfile_dir")}/{time_id}' | ||
|
|
||
| LOG.info(f'Found {autos.count} files to process. Screens name are like "{time_id}_autoX"') | ||
| LOG.info('To switch screens detach from this screen via "<ctrl>+a d".') |
There was a problem hiding this comment.
We should add a note here that these shortcuts adhere to the users .screenrc, so they might be different.
There was a problem hiding this comment.
I just added a line, that you can modify the behaviour via screenrc.
This is to allow global activation of the bash completion.
This PR merges all the changes from https://github.com/vanadinit/automatix_cmd
I first considered rewriting or just taking parts of the code, but there was also some major refactoring of the main code so I felt like I would do all the work a second time. Feel free to comment and make suggestions for improvements.