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

Namsonx/task/stabi branch #369

Merged
merged 30 commits into from
Nov 15, 2024
Merged

Conversation

namsonx
Copy link
Collaborator

@namsonx namsonx commented Oct 30, 2024

No description provided.

@@ -86,4 +86,7 @@
- Prevented side effects of token string in error messages log\newline
- Checked absolute path when overwriting parameter}

\historyversiondate{0.8.2}{10/2024}
\historychange{- Enhanced import JSON file feature which allows dynamic path of imported file}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Son,

please rephrase to:

Enabled dynamic paths for imported JSON files (based on dollar operator expressions)

Copy link
Owner

@test-fullautomation test-fullautomation left a comment

Choose a reason for hiding this comment

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

Hi Son,
please check my finding,
Thank you,
Thomas

else:
sJson = re.sub(r'"\s*\[\s*import\s*\]\s*":\s*"[${]+[^"]+"', checkDynamicImport[0], sJson)
self.__preCheckJsonFile(sJson, CJSONDecoder)
self.bDynamicImport = False

Choose a reason for hiding this comment

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

Hi Son,
this won't work most likely.
Once bDynamicImport is set this will affect all later recursions. It will be always set.
The idea to use a instance-variable here is strange for me. It is a matter of each single import if there is a dynamic import. Why should the whole jsonprerpro instance know this for a specific import.
Please check the design.

Code to demostrate issue:

class MyClass:
    def __init__(self):
        # Instanzvariable: hasImport, standardmäßig False
        self.hasImport = False

    def recursive_method(self, count=0):
        # Status von hasImport in der Konsole loggen
        print(f"Rekursionstiefe: {count}, hasImport: {self.hasImport}")

        # Rekursive Bedingung
        if count < 10:
            # Letzte Aktion in der Rekursion
            if count == 4:
                self.hasImport = True
            
            # Rekursiver Aufruf
            self.recursive_method(count + 1)

        self.hasImport = False


# Beispiel zur Verwendung der Klasse
my_instance = MyClass()
my_instance.recursive_method()

Thank you,
Thomas

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@namsonx namsonx Oct 31, 2024

Choose a reason for hiding this comment

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

Hello Thomas,

We handle import files in the hook method of json.loads()

            oJson = json.loads(sJsonDataUpdated,
                               cls=CJSONDecoder,
                               object_pairs_hook=self.__processImportFiles)

then nested parameters will be detected and replaced by the method oJson, bNested = self.__updateAndReplaceNestedParam(oJson)
With this machenism, we could not get values of parameters while __processImportFiles.

I designed to create the method __preCheckJsonFile() to load and replace parameters in dynamic paths of import files.
While handling JSON configuration file with __preCheckJsonFile(), if it detects dynamic import in JSON config file, the flag bDynamicImport sets True until __preCheckJsonFile() finished.
The flag bDynamicImport is set to ensure during pre-check phase if there are parameters which are not available (due to these parameters belong to dynamic import files), these exceptions will be bypassed.

After pre-check phase is completed, the flag bDynamicImport is set False, and all dynamic paths of import files are solved.
Then we call

            oJson = json.loads(sJsonDataUpdated,
                               cls=CJSONDecoder,
                               object_pairs_hook=self.__processImportFiles)

and process as normal.

Thank you,
Son

Copy link
Owner

@test-fullautomation test-fullautomation Nov 1, 2024

Choose a reason for hiding this comment

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

Hi Son,
thank you for your answer.
I have still concerns. The nature of recursion is that once your self.bDynamicImport is set, it will be set for all further recursions due to nested imports. My sample code shows this vey nice.
@HolQue : Can you please support with a test-case for this.
Thank you,
Thomas

Copy link
Collaborator

Choose a reason for hiding this comment

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

Test files already available: #376 (comment)

Tests soon.

Copy link
Collaborator

@HolQue HolQue left a comment

Choose a reason for hiding this comment

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

@HolQue
Copy link
Collaborator

HolQue commented Nov 15, 2024

Hi Son,

{
    "[import]" : 123
}

'The value of [import] parameter must be 'str' but receiving the value '123''

Please rephrase to:

The [import] key requires a value of type 'str', but the type is 'int'.

Copy link
Owner

@test-fullautomation test-fullautomation left a comment

Choose a reason for hiding this comment

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

Hi Son,
very good!
Thank you,
Thomas

@test-fullautomation test-fullautomation added enhancement New feature or request 0.13.1 labels Nov 15, 2024
@test-fullautomation test-fullautomation added this to the 0.13.1 milestone Nov 15, 2024
@test-fullautomation test-fullautomation merged commit 86dd7af into develop Nov 15, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.13.1 enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

3 participants