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

Add strict subvariable syntax handler #465

Merged
merged 5 commits into from
Aug 1, 2024

Conversation

aless10
Copy link
Contributor

@aless10 aless10 commented Jul 31, 2024

Add strict_subvariable_syntax flag handler for system script + tests

@aless10 aless10 requested a review from jjdelc July 31, 2024 14:10
scrunch/accounts.py Show resolved Hide resolved
scrunch/datasets.py Show resolved Hide resolved
def format_request_url(self, execute, strict_subvariable_syntax=None):
strict_subvariable_syntax_flag = self.get_default_syntax_flag(strict_subvariable_syntax)
if strict_subvariable_syntax_flag:
execute.self += "?strict_subvariable_syntax=true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you will be mutating the self attribute of the execute resource. I think that this better returns the desired URL instead of altering the instance.

Because when you execute a script for the 2nd time this will add again this string incorrectly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not completely true. I am not mutating the self.resource instance, but just the local execute.
Every time you do self.resource.something there is a GET call to the resource o get the attribute.
So the self.resource instance is not modified by this, I am just updating the local object that is the result of the GET
So here

execute = self.resource.execute
I am fetching the execute object, and I am modifing this one that is the result of this call https://github.com/Crunch-io/pycrunch/blob/9bb073368293f1e84843a53bdc0cfccd1c90787e/src/pycrunch/elements.py#L154
Next time, next script, I am going to redo the request to the same self.resource object, that has not being modified.
Let me know if that works for you. Otherwise I'll think of another way of doing this. But I just wanted to use the pycrunch interface as per the DatasetsScripts

scrunch/accounts.py Outdated Show resolved Hide resolved
try:
execute = self.resource.execute
self.format_request_url(execute, strict_subvariable_syntax)
return execute.post(payload)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see why you had to mutate the instance, I think that here it's best to use a raw request, see how this pycrunch .post() is implemented and you'll do a self.resource.session.post(execute_url. ....).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained above, I did not mutate the instance, so we should be safe

@aless10 aless10 requested a review from jjdelc August 1, 2024 08:58
@aless10 aless10 merged commit db4dfea into master Aug 1, 2024
5 checks passed
@aless10 aless10 deleted the add-strict-subvariable-syntax-handler branch August 1, 2024 15:32
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