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

Change naming of start and end time #46

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Conversation

fhenneke
Copy link
Contributor

This PR closes #45.

from StartTime to start_time
from EndTime to end_time
@fhenneke fhenneke requested a review from harisang September 20, 2024 07:36
Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Seems legit.

Although, just to clarify, the casing convention given to QueryParameters was intended not to conflict or confuse with that of table_names and result_columns.

this caused an issue with tests
Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
pypi/[email protected] environment, eval, filesystem, network, shell, unsafe 0 45.7 MB ambv, guido, hauntsaninja, ...8 more

🚮 Removed packages: pypi/[email protected]

View full report↗︎

@@ -18,7 +18,8 @@ def test_query_runner(self, mocked_post):
query = load_config(filepath("v2-test-data.yaml")).query
dune = DuneClient(os.environ["DUNE_API_KEY"])
slack_client = BasicSlackClient(token="Fake Token", channel="Fake Channel")
query_runner = QueryRunner(query, dune, slack_client)
ping_frequency = 10

Choose a reason for hiding this comment

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

what exactly is this adding? Noticed this is in the tests so not clear what the purpose of this change is

Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is that the default ping frequency was lowered to 1 second in the client package and so the test was producing an unnecessary number of logs. Back when this was built the default ping frequency was 10. But dune has faster engines now so it was lowered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just that (on my machine) running the tests was not possible since ping_frequency was a required parameter.

I did not want to look at updating/checking dependencies so just provided a value which seemed reasonable.

@fhenneke
Copy link
Contributor Author

fhenneke commented Sep 20, 2024

Although, just to clarify, the casing convention given to QueryParameters was intended not to conflict or confuse with that of table_names and result_columns.

It looks like a reasonable approach, which is however not compatible with how Dune set up their new query view feature. See limitation 5 here.

Maybe camel case would have worked as well.
(Edit: just tested it and camel case also does not work

parameter key 'testParameter' has upper case characters. Please use keys with only lower case characters 

So the examples in the Dune documentation are wrong/misleading)

@fhenneke fhenneke merged commit c7f25ef into main Sep 20, 2024
4 checks passed
@fhenneke fhenneke deleted the change_parameter_naming branch September 20, 2024 09:14
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.

Change naming of time parameters
3 participants