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

Update group_id syntax #63

Merged
merged 7 commits into from
Jun 19, 2024
Merged

Update group_id syntax #63

merged 7 commits into from
Jun 19, 2024

Conversation

JulienPeloton
Copy link
Collaborator

@JulienPeloton JulienPeloton commented Jun 18, 2024

Closes #62

This PR updates the syntax for the group.id parameter for Kafka in fink-client. I also updated the required version of fink-client & minimum version of Python.

@JulienPeloton JulienPeloton added the bug Something isn't working label Jun 18, 2024
@JulienPeloton JulienPeloton requested a review from phycodurus June 18, 2024 11:05
@@ -88,7 +88,7 @@ def listen(self):
while poll_number < int(self.max_poll_number):
try:
logger.info(
"FinkAlertStream.listen opening stream: {} with group_id: {} (call number: {})".format(
"FinkAlertStream.listen opening stream: {} with group.id: {} (call number: {})".format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Julien, thanks for keeping tom_fink up-to-date. How about using an f-string here?

logger.info(f"FinkAlertStream.listen opening stream {self.url} with group.id {self.group_id} (call number: {poll_number})")

or this also works if the line gets too long:

logger.info(f"FinkAlertStream.listen opening stream {self.url}"
            f" with group.id {self.group_id} (call number: {poll_number})")

If you like that syntax, there are other opportunities to convert from "".format() to f-string syntax around lines 138-143 and 156-158.

Thanks again!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @phycodurus for the review -- I converted to f-string when it was simple enough -- but I prefer keeping the .format syntax when it involves dictionaries (clearer to my opinion).

@JulienPeloton
Copy link
Collaborator Author

@phycodurus I'll let you merge (I'm no more authorized to do it?)

@jchate6
Copy link
Contributor

jchate6 commented Jun 18, 2024

@phycodurus I'll let you merge (I'm no more authorized to do it?)

Hi @JulienPeloton!
Sorry this was my fault. I'm going through adding some checks and security to our TOM Repositories.
I was intending to just make it have a check requiring approval by a reviewer before someone could push a PR to main.
I think I've fixed this now, and you should be able to push.

If you are still having issues, let me know and I'll put it back how it was before.

@JulienPeloton
Copy link
Collaborator Author

Thank you @jchate6 -- the current setup is perfect!

@JulienPeloton JulienPeloton merged commit a290c19 into main Jun 19, 2024
22 checks passed
@jchate6 jchate6 deleted the issue/62/bug branch June 26, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[bug] group_id is not recognised anymore
3 participants