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

JobKey, TriggerKey and Key<T> are now immutable #1375

Draft
wants to merge 1 commit into
base: v4
Choose a base branch
from

Conversation

drieseng
Copy link
Contributor

Made JobKey, TriggerKey and Key<T> immutable, and therefore Key<T> no longer defines a default protected ctor.

The implementation of Equals(object? obj) has simplified due to this change, but the positive effect on micro-benchmarks (that I added in this PR) is within noise range.

Remove protected default ctor of Key<T>.
@drieseng drieseng marked this pull request as draft October 27, 2021 21:52
@drieseng
Copy link
Contributor Author

Don't you just love serialization magic ... :(

@drieseng
Copy link
Contributor Author

@lahma I'm not asking this specifically for this PR (but your answer could drive what we decide to do with this one), but is it ok to achieve backward compatibility for JSON serialization through the introduction of a custom JsonConverter (which would have to be plugged into JsonObjectSerializer.CreateSerializerSettings()?

Meaning, is it ok to make changes (like those in this PR) if we can support and generate the same on-the-wire (JSON) format?

@lahma
Copy link
Member

lahma commented Oct 28, 2021

I'm not sure if I'm answering your question, but as long as we can deserialize data persisted to database we should be OK. This includes, triggers, jobs and calendars.

@drieseng
Copy link
Contributor Author

@lahma Have you already shipped a version with nullable annotations? We're not very "honest" about the nullability characteristics on several properties.

@lahma
Copy link
Member

lahma commented Oct 28, 2021

I think I have: #866

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.

None yet

2 participants