Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#174 replace language container stuff with pec and slc pytest plugin #182
#174 replace language container stuff with pec and slc pytest plugin #182
Changes from 60 commits
90125c1
e225ece
231ce11
b31d203
b9c7621
ce76e79
d868c64
9d2931b
2d7cb8a
4d8772f
a264987
76a1c2f
bcb7a57
84d4eb4
580629a
d787837
b41e217
306fc3f
6f72d00
9bb5cef
97458ea
2d91514
6837def
e900211
8cb8a7b
829c480
b03f819
4170a3a
67e50b9
6501ac1
9ba9727
d93646f
fd42667
2790467
695e46c
15dd59a
6f84880
504a592
d7ae37e
fed4986
3e662e4
b1fac53
c72cbdf
4285491
172be2e
b16a92d
b1f03fa
64d853b
fed53c8
8191616
a30ab7a
a8cf3ce
92a090e
0242e26
3fb91d9
d8e868c
8c5c6d3
fa64a89
32e44ab
3fc3678
e8f57ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I preferred passing the
pyexasol_connection
instead of the individual parametersdsn
,user
andpassword
as the tests are using the fixturepyexasol_connection
anyway and extracting the former individual parameters seemed cumbersome to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the pyexasol Connection you have the constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor does not call
save_aaf_query_loop_lua_script()
, norscripts_deployer.deploy_scripts()
.So in the end I still prefer changing the signature.
Alternatively we could use a different name and keep the old method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For explanation, the run is specifically for the CLI and for nothing else. Other code should call deploy_scripts directly and should use an already generated Lua script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found 3 places calling
ScriptsDeployer.run()
:The current implementation of the run() method is
As P2 and P3 both are using flag
develop=True
I will use the following code in order to use the constructor:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be ok for now. We could also think about moving the save_aaf_query_loop_lua_script call into its own fixture