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 max_phase as an option #13

Merged
merged 8 commits into from
Jan 30, 2024
Merged

Added max_phase as an option #13

merged 8 commits into from
Jan 30, 2024

Conversation

jhylin
Copy link
Contributor

@jhylin jhylin commented Jan 29, 2024

This PR is related to this issue #1, many thanks.

@cthoyt
Copy link
Owner

cthoyt commented Jan 29, 2024

In your post, you just had it return the max phase. Here, it look like you're adding a filter. I think a different way to do this is to just add a boolean flag for returning max phase and then format it in to the SELECT part of the query instead of the WHERE part

Copy link
Contributor Author

@jhylin jhylin left a comment

Choose a reason for hiding this comment

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

Oops yes, that was acting like a filter, I've changed it into a boolean flag (leaving it as False as default, and can be toggled on if set to True), and added it into the string code section, and also the actual SQL part. Is this better?

1. Just check bools directly in if/else
2. No need for f-string
3. Stick the format in the middle to get around the trailing comma issue.
@cthoyt
Copy link
Owner

cthoyt commented Jan 29, 2024

@jhylin yes! I made a minor update in 72cd48a but this should do it. Did you test it works locally?

@jhylin
Copy link
Contributor Author

jhylin commented Jan 30, 2024

Yes, I just did a unittest on the latest code (I've pushed the test_queries.py in the /test folder), and it worked. The only thing was (very likely due to my lack of experience in doing unittest), I had to temporarily make target_id as Optional[str], and the test passed - unsure how I would resolve the error message regarding "missing 1 positional statement for target_id" in the unittest if I didn't make it optional?

@cthoyt cthoyt merged commit 2e0999c into cthoyt:main Jan 30, 2024
5 checks passed
@cthoyt
Copy link
Owner

cthoyt commented Jan 30, 2024

@jhylin the trick was that you were missing the target_id argument - you just had to pass a string. I did a bit of re-org and merged this. Thanks! Will make a release momentarily.

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