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 more detailed state for property. #367

Closed
wants to merge 2 commits into from
Closed

Add more detailed state for property. #367

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 21, 2021

This PR aggregates all the "check time" state of a property into a single structure. This will hopefully make adding features like #341 easier, and I think it's much easier to see at a glance what's happening internally by looking at the new PropertyState module.

/cc @TysonMN @dharmaturtle

@TysonMN
Copy link
Member

TysonMN commented Sep 21, 2021

Can we merge #336 first? I don't want to fix merge conflicts.

@dharmaturtle
Copy link
Member

I like this a lot.

@ghost
Copy link
Author

ghost commented Sep 22, 2021

Can we merge #336 first? I don't want to fix merge conflicts.

Absolutely. Are we still waiting on a Fable issue there?

I like this a lot.

👍

@TysonMN
Copy link
Member

TysonMN commented Sep 23, 2021

Can we merge #336 first? I don't want to fix merge conflicts.

Absolutely. Are we still waiting on a Fable issue there?

We are primarily waiting on PR #364 to be reviewed and completed. After that, we could merge PR #336 with understanding that the API needs some work, but maybe that is ok since improving the API around Property is what issue #365, PR #346, and this PR are intended to do.

And the issue with Fable is minor. The simple fix is to comment out one of the assertions. I think it would be better to have that assertion, but it isn't the only assertion, and we can wait to comment back in this second assertion once issue with Fable is resolved.

@ghost ghost closed this by deleting the head repository Oct 27, 2022
This pull request was closed.
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