Skip to content

Conversation

@vicchi
Copy link
Collaborator

@vicchi vicchi commented Jan 18, 2025

@long2ice This PR is a maintenance release for v0.2.3. It's mainly to nuke most outstanding dependabot PRs, to ensure the tests pass and to make sure I've grokked the way the project should be maintained and updated.

I'm happy to merge, tag and release this version but would really appreciate your feedback before I do.

vicchi added 23 commits January 17, 2025 16:19
… coder which rely on decoding the (currently unsupported) custom non-JSON data types
@vicchi vicchi added this to the Version v0.2.3 milestone Jan 18, 2025
@github-actions github-actions bot added the changelog-provided PR includes a towncrier changelog entry label Jan 18, 2025
@vicchi vicchi requested a review from long2ice January 18, 2025 11:50
@vicchi
Copy link
Collaborator Author

vicchi commented Feb 6, 2025

@long2ice Gentle nudge?

Copy link

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @vicchi . A few comments.

Just to clarify do you have commit rights for this project or not?

Regarding tests, yes it's unfortunate that they are failing on main. Given the situation I agree that skipping the failing tests to make main green is probably best. v0.2.2 was released with those failing tests, so at least this shouldn't break anything backward compatibility wise.

I'm trying to make it work with modern version of Python, and indeed there are issue, so a new release would be very welcome.

The rest of the changes LGTM.


async def get_with_ttl(self, key: str) -> Tuple[int, Optional[bytes]]:
response = await self.client.get_item(TableName=self.table_name, Key={"key": {"S": key}})
if self.client:
Copy link

Choose a reason for hiding this comment

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

Better to have

if not self.client:
    return 0, None

to avoid deeply nested ifs and changing the indention of everthing below.

- (dependabot) Bump actions/upload-artifact from 3 to 4 (#360) [#360](https://github.com/long2ice/fastapi-cache/issues/360)
- (dependabot) Bump actions/cache from 3 to 4 (#378) [#378](https://github.com/long2ice/fastapi-cache/issues/378)
- (dependabot) Bump dependabot/fetch-metadata from 1 to 2 (#464) [#464](https://github.com/long2ice/fastapi-cache/issues/464)
- (dependabot) Bump tox from 4.20.0 to 4.23.2 (#466) [#466](https://github.com/long2ice/fastapi-cache/issues/466)
Copy link

@rth rth Apr 17, 2025

Choose a reason for hiding this comment

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

It's a dev dependency, users wouldn't care. Same about CI related updates (actions/download-artifact etc). It might be better to exclude those from the changelog.

invalid = b'{"name": "incomplete"}'
with pytest.raises(ValidationError):
JsonCoder.decode_as_type(invalid, type_=PDItem)
# def test_json_coder_validation_error() -> None:
Copy link

Choose a reason for hiding this comment

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

Maybe better to mark it as a known failure rather than commenting with @pytest.mark.xfail(reason="See issue #460")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-provided PR includes a towncrier changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants