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

feat: add cap status to parsed event #2

Closed

Conversation

antonanders
Copy link
Contributor

minimal change to make filtering work.
See jdemaeyer/brightsky#166

this is needed to drop status=test alerts.
@jdemaeyer
Copy link
Owner

Hi @antonanders, thanks a lot for the PR!

I'm attaching a test data file containing one of the test alerts, could you add that to tests/data and a small test checking that parsing it does indeed output a record with record['status'] == 'test'?

Also - and I realize this is super nitpicky 😅 - this repository uses single quotes for tokens / short strings, could you adjust that?

Z_CAP_with_test.zip

@antonanders
Copy link
Contributor Author

Hi @jdemaeyer,

thanks for pointing that out! Python is pretty new to me, so I hope, the changes are OK. :)

@jdemaeyer
Copy link
Owner

No worries at all, I couldn't be more grateful that you're submitting a PR :) I've added two smaller comments in the hope you're not too annoyed yet, but after that we should be good to go!

@antonanders
Copy link
Contributor Author

@jdemaeyer Thanks!
Could you point me to the comments? I don't see them right now in the "File changed" tab or anywhere else.

def test_cap_parser(data_dir):
test_cap_parser_actual_status(data_dir)
test_cap_parser_test_status(data_dir)
Copy link
Owner

Choose a reason for hiding this comment

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

pytest automatically picks up all functions prefixed with test_ , so we don't need this wrapper function :)

'onset': datetime.datetime(2024, 4, 23, 10, 21, tzinfo=datetime.timezone(datetime.timedelta(seconds=7200))), # noqa
'expires': datetime.datetime(2024, 4, 23, 12, 0, tzinfo=datetime.timezone(datetime.timedelta(seconds=7200))), # noqa
}

Copy link
Owner

Choose a reason for hiding this comment

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

Our old test already covers parsing for all other fields, so probably just assert records[0]['status'] == 'test' is enough here?

@jdemaeyer
Copy link
Owner

Ah my bad, should be there now :)

@jdemaeyer
Copy link
Owner

Brilliant, merged with minor styling updates via 24759b1, thanks a lot! :) I'll add the changes to jdemaeyer/brightsky after resolving #3.

@jdemaeyer jdemaeyer closed this May 2, 2024
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