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

Updated docs for onCheck properties #215

Merged
merged 1 commit into from Aug 23, 2022
Merged

Updated docs for onCheck properties #215

merged 1 commit into from Aug 23, 2022

Conversation

shanshin
Copy link
Collaborator

Resolves #213

@shanshin shanshin requested a review from qwwdfsad August 22, 2022 15:46
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

It seems to be an incomplete fix -- it's easy to misunderstand this documentation and still misuse API.

I suggest throwing a configuration error if onCheck is explicitly set for projects without check task

@shanshin
Copy link
Collaborator Author

I suggest throwing a configuration error if onCheck is explicitly set for projects without check ta

For this checking with it is necessary to slightly change the API - the default value will now be "no value".

@qwwdfsad
Copy link
Member

For this checking with it is necessary to slightly change the API - the default value will now be "no value".

It is not, because no value and false are identical semantically anyway.
Our goal here is to provide fail-fast mechanism for the common scenario where user misconfigures their project by explicitly invoking onCheck(true).

It is perfectly fine not to fail on onCheck(false) as this behaviour is indistinguishable from setting nothing at all

@shanshin
Copy link
Collaborator Author

  1. as far as I know, it is not possible to add a listener to Property.set(true)
  2. verify tasks true by default now
  3. when checking, I will not distinguish the default true from the one set explicitly
  4. if I replace the onCheck property with a function with an immediate check for the presence of the check task, then this may work unstable, because the task can be created after assigning a value (eq in afterEvaluate)

@shanshin shanshin requested a review from qwwdfsad August 23, 2022 10:34
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

:okayface:

@shanshin shanshin merged commit a0ff8d9 into main Aug 23, 2022
@shanshin shanshin deleted the on-check-docs branch August 23, 2022 13:32
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.

with onCheck.set(true) merged tasks are not launched
2 participants