Skip to content

Conversation

@Mantisus
Copy link
Collaborator

@Mantisus Mantisus commented Dec 6, 2024

Description

  • Removing json_ and order_no from Request which is a model of external API. Creating an internal model InternalRequest to work inside _request_queue_client
  • Add new tests

Issues

Checklist

  • CI passed

@Mantisus Mantisus changed the title Refactor request refactor: delete json_ and order_no from Request Dec 6, 2024
@Mantisus Mantisus changed the title refactor: delete json_ and order_no from Request refactor: remove json_ and order_no from Request Dec 6, 2024
@Mantisus Mantisus marked this pull request as draft December 6, 2024 11:52
@Mantisus Mantisus added t-tooling Issues with this label are in the ownership of the tooling team. debt Code quality improvement or decrease of technical debt. labels Dec 6, 2024
@Mantisus Mantisus marked this pull request as ready for review December 9, 2024 14:33
Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

Good job, it is much better now! And I have a few things...

@Mantisus Mantisus changed the title refactor: remove json_ and order_no from Request refactor!: remove json_ and order_no from Request Dec 11, 2024
@Mantisus Mantisus requested review from janbuchar and vdusek December 12, 2024 17:42
@Mantisus Mantisus requested a review from janbuchar December 13, 2024 15:30

payload: Annotated[
HttpPayload | None,
BeforeValidator(lambda v: v.encode() if isinstance(v, str) else v),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the BeforeValidator still needed when loading an old request file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. We need both so that payload is correctly serialized and deserialized to a file.
Updated the e2e test.

Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

Amazing job, thanks.

@janbuchar janbuchar self-requested a review December 16, 2024 16:22
Copy link
Collaborator

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

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

Nice one!

@vdusek vdusek merged commit 5381d13 into apify:master Dec 16, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debt Code quality improvement or decrease of technical debt. t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove json_ and order_no from Request

3 participants