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.

Sign up for GitHub

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

Jump to bottom

Improved integration with Global ID #928

Merged
merged 1 commit into from
Sep 1, 2024
Merged

Improved integration with Global ID #928

merged 1 commit into from
Sep 1, 2024

Conversation

Alexander-Senko
Copy link
Collaborator

@Alexander-Senko Alexander-Senko commented Feb 6, 2024 •
edited
Loading

Description

Implementing Decorator.find improves compatibility with Global ID and allows one to use decorated objects in background jobs seamlessly.

Testing

  1. Decorate an object.
  2. Serialize the decorated object with Global ID or pass it to a job.
  3. Deserialize the object.
  4. Check if it's the same one and is still decorated.

To-Dos

  • tests
  • documentation

References

Alexander-Senko added a commit to Alexander-Senko/draper that referenced this pull request Feb 6, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
d38e92a 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves drapergem#910.
Requires drapergem#928.
@Alexander-Senko Alexander-Senko mentioned this pull request Feb 6, 2024
Merged
2 tasks
@Alexander-Senko Alexander-Senko self-assigned this Aug 20, 2024
y-yagi
y-yagi requested changes Aug 21, 2024
View reviewed changes
Copy link
Collaborator

@y-yagi y-yagi left a comment

Choose a reason for hiding this comment

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

Could you rebase with the master and run CI?
I tried to run the test on my local with this PR, but some tests failed.

@Alexander-Senko Alexander-Senko removed the request for review from seanlinsley August 22, 2024 08:47
Alexander-Senko added a commit to Alexander-Senko/draper that referenced this pull request Aug 30, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
4f119e0 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves drapergem#910.
Requires drapergem#928.
Alexander-Senko added a commit to Alexander-Senko/draper that referenced this pull request Aug 30, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
f62609a 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves drapergem#910.
Requires drapergem#928.
Alexander-Senko added a commit to Alexander-Senko/draper that referenced this pull request Aug 30, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
7009fc3 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves drapergem#910.
Requires drapergem#928.
Alexander-Senko added a commit to Alexander-Senko/draper that referenced this pull request Aug 30, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
7611403 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves drapergem#910.
Requires drapergem#928.
@Alexander-Senko Alexander-Senko added this to the 4.0.3 milestone Aug 31, 2024
@y-yagi
Copy link
Collaborator

y-yagi commented Sep 1, 2024

@Alexander-Senko
This patch itself looks good to me. But, why did you add 4.0.3 tag to this PR? This PR brings a little breaking change. I think it's not a good idea to introduce a breaking change in a patch version update.

@Alexander-Senko
Copy link
Collaborator Author

Alexander-Senko commented Sep 1, 2024 •
edited
Loading

This patch itself looks good to me.

Thus, could you approve it, please?

But, why did you add 4.0.3 tag to this PR?

I don't have a strict opinion on that. You can reattach it to whatever milestone you like. It does nothing for now except organizing the issues until we get access to the gem.

I've just started organizing the issues and introduced new milestones. Any help on that is appreciated.

This PR brings a little breaking change. I think it's not a good idea to introduce a breaking change in a patch version update.

I see it a bit differently — as a know bug, but not an official API 🤔 Patch releases may break hacks and workarounds if they don't follow official API, may not they?

@y-yagi
Copy link
Collaborator

y-yagi commented Sep 1, 2024

I don't have a strict opinion on that. You can reattach it to whatever milestone you like. It does nothing for now except organizing the issues until we get access to the gem.
I've just started organizing the issues and introduced new milestones. Any help on that is appreciated.

Thanks for your detailed explanation. I understand what the purpose.

I see it a bit differently — as a know bug, but not an official API 🤔 Patch releases may break hacks and workarounds if they don't follow official API, may not they?

I don't think so. This is documented behavior, you have changed the document, right?
We shouldn't change the documented behavior in changing patch versions and minor versions(as we follows the semantic versioning strictly).

y-yagi
y-yagi approved these changes Sep 1, 2024
View reviewed changes
Copy link
Collaborator

@y-yagi y-yagi left a comment

Choose a reason for hiding this comment

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

LGTM!

@y-yagi y-yagi modified the milestones: 4.0.3, 5.0 Sep 1, 2024
@Alexander-Senko
Improved integration with Global ID
430d08f 
Implementing `Decorator.find` improves compatibility with Global ID and
allows one to use decorated objects in jobs seamlessly.

Resolves drapergem#663.
Improves drapergem#817.
@Alexander-Senko Alexander-Senko merged commit 4d06805 into drapergem:master Sep 1, 2024
8 checks passed
Alexander-Senko added a commit to Alexander-Senko/draper that referenced this pull request Sep 2, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
4f4008c 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves drapergem#910.
Requires drapergem#928.
Alexander-Senko added a commit that referenced this pull request Sep 3, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
a1439ff 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves #910.
Requires #928.
Alexander-Senko added a commit to Alexander-Senko/draper that referenced this pull request Sep 12, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
3b4e98d 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves drapergem#910.
Requires drapergem#928.
Alexander-Senko added a commit that referenced this pull request Sep 26, 2024
@Alexander-Senko
Integration with Turbo::Broadcastable
28c134d 
Overriding defaults for Turbo broadcast jobs allows one to get decorated
objects in model partials by default.

Resolves #910.
Requires #928.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y-yagi y-yagi y-yagi approved these changes

Assignees

@Alexander-Senko Alexander-Senko

Labels
None yet
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.

ActiveJob Integration
2 participants
@Alexander-Senko @y-yagi