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

Add type annotations for test_reporting #54

Closed

Conversation

Dhwaniartefact
Copy link
Contributor

No description provided.

Comment on lines 66 to 67
report: Optional[Report],
report_url: Optional[str],
Copy link
Member

Choose a reason for hiding this comment

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

Why are these Optional?

Copy link
Contributor Author

@Dhwaniartefact Dhwaniartefact Oct 10, 2024

Choose a reason for hiding this comment

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

Some tests are setting as report=None and report_url=None.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think if we focus on the actual use from the fixity package and adjust the tests accordingly if needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya let me adjust the tests which focus on the actual use of the codes.

Comment on lines 19 to 20
start_time: Optional[datetime],
report_url: Optional[str],
Copy link
Member

Choose a reason for hiding this comment

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

Why are these Optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above.

Copy link
Member

Choose a reason for hiding this comment

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

Same argument as above 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one. :) I got your point. Let me adjust the tests.

@Dhwaniartefact Dhwaniartefact force-pushed the dev/issue-add-type-hints-test-reporting branch from ce500f8 to 28c4c83 Compare October 11, 2024 15:57
aip: str,
start_time: datetime,
report_url: str,
report_auth: str = (),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this typed as str when it expects a tuple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah yes, I missed to see this.

@Dhwaniartefact Dhwaniartefact force-pushed the dev/issue-add-type-hints-test-reporting branch 2 times, most recently from 8ca032e to cc741e7 Compare October 11, 2024 16:29
@Dhwaniartefact Dhwaniartefact force-pushed the dev/issue-add-type-hints-test-reporting branch from cc741e7 to 00b205f Compare October 12, 2024 00:18
@Dhwaniartefact
Copy link
Contributor Author

We will annotate test_reporting in future PRs.

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