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

Stop empty messages being sent and log warning gracefully #267

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

RedProkofiev
Copy link
Contributor

Resolves #261

Empty messages and files render as empty lines through Dirq but a simple len() > 0 check means that empty files will now return None, and some changes to send_all now gracefully handle None input to log a warning.

@RedProkofiev
Copy link
Contributor Author

Codeclimate continually fails due to the extra if statement needed for the log/warning element. Unsure if possible to fix neatly without a function rewrite, so I've put a truly ugly fix here for review as a reminder to rewrite it if needed.

@RedProkofiev RedProkofiev marked this pull request as ready for review September 19, 2023 15:41
@RedProkofiev RedProkofiev requested a review from a team as a code owner September 19, 2023 15:41
@tofu-rocketry
Copy link
Member

You've got one of the LDAP commits in here.

@gregcorbett
Copy link
Member

Codeclimate continually fails due to the extra if statement needed for the log/warning element. Unsure if possible to fix neatly without a function rewrite, so I've put a truly ugly fix here for review as a reminder to rewrite it if needed.

I would go for that fix / function re-write. Something like the following would work, and that would avoid the need for log_string[0] == 'M'. right?

if argo_id is not None:
    log.info("Sent..."
else:
    log.warning("Message..."

@RedProkofiev
Copy link
Contributor Author

it wouldn't work: Argo_ID is out of scope in that context because of the way you're doing the logs: the argo_id is used only for constructing the log string. So the log string is the only thing passed out of scope
I think it'll require a rewrite to make the warning work neatly. Also, the if...else statement would still fail complexity requirements under 15, I've tried!!!!

Should I just try and rewrite the whole thing?

@RedProkofiev
Copy link
Contributor Author

it wouldn't work: Argo_ID is out of scope in that context because of the way you're doing the logs: the argo_id is used only for constructing the log string. So the log string is the only thing passed out of scope I think it'll require a rewrite to make the warning work neatly. Also, the if...else statement would still fail complexity requirements under 15, I've tried!!!!

Should I just try and rewrite the whole thing?

I just realised I'm being very silly here! sorry, ignore me, it's totally in scope. However we're resolving to instead just have three separate log message/warning elements rather than upping the complexity with another if...else block. :)

@tofu-rocketry tofu-rocketry self-assigned this Oct 6, 2023
@tofu-rocketry tofu-rocketry added this to the 3.5.0 milestone Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

AMS sending silently ignores empty messages
3 participants