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

🚀 Added new functionality to saprfc source regarding where statement #899

Merged
merged 7 commits into from
May 14, 2024

Conversation

adrian-wojcik
Copy link
Contributor

Summary

Adding new functionality to saprfc source regarding where statement and dynamic part of after 70 characters

Importance

It is important to prevent passing sql query without white spaces between operators and WHERE statement and also raise error when dynamic query is detected after 70 characters after WHERE

Checklist

This PR:

  • [X ] follows the guidelines laid out in CONTRIBUTING.md
  • links relevant issue(s)
  • adds/updates tests (if appropriate)
  • [X ] adds/updates docstrings (if appropriate)
  • [ X] adds an entry in CHANGELOG.md

CHANGELOG.md Show resolved Hide resolved
@adrian-wojcik adrian-wojcik requested a review from trymzet May 9, 2024 08:56
Copy link
Contributor

@trymzet trymzet left a comment

Choose a reason for hiding this comment

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

I don't have anything to say about the code itself, but why isn't this logic added to _get_where_condition()?

@adrian-wojcik
Copy link
Contributor Author

I don't have anything to say about the code itself, but why isn't this logic added to _get_where_condition()?

Because, in method query() variable sql is passed to multiple methods, so if we will add this functionality to the _get_where_condition() other functions will fail. So we decide that better approach is to take raw sql string at the beginning and refactor it before passing to other methods

@trymzet
Copy link
Contributor

trymzet commented May 9, 2024

Ah ok, I thought this is only sanitizing the WHERE clause, didn't think this would impact the rest of the parsing (I assume you mean that the rest of the parsing in extract_values() is failing, right?).

In this case, I would only recommend adding a longer docstring with some context as to why this is needed because this is hard to understand just from the function's name and code.

@adrian-wojcik
Copy link
Contributor Author

Ah ok, I thought this is only sanitizing the WHERE clause, didn't think this would impact the rest of the parsing (I assume you mean that the rest of the parsing in extract_values() is failing, right?).

In this case, I would only recommend adding a longer docstring with some context as to why this is needed because this is hard to understand just from the function's name and code.

Yes, exactly. I will add soon more specific docstring to function.

Copy link
Contributor

@trymzet trymzet left a comment

Choose a reason for hiding this comment

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

Looks good.

@adrian-wojcik just a tip: you can use tools like Grammarly or gen AI chatbots like ChatGPT to fix the English grammar in docstrings/comments.

@trymzet
Copy link
Contributor

trymzet commented May 14, 2024

@adrian-wojcik 1 more thing -- adding tests, even if they're only integration tests, is highly recommended, since without them it will be a lot harder to 1) know if this actually works, 2) debug things if this logic goes wrong at some point (and I see the logic in the connector is already quite complicated -- it's basically a colossus with legs of clay).

@trymzet trymzet merged commit 698eac3 into dyvenia:2.0 May 14, 2024
2 of 3 checks passed
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.

3 participants