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 PreferenceStore interface #658

Merged
merged 1 commit into from Jan 15, 2019

Conversation

Projects
None yet
2 participants
@cdupuis
Copy link
Contributor

commented Jan 15, 2019

No description provided.

@ddgenome
Copy link
Member

left a comment

Looks good, this will be nice functionality. My only question is around backward compatibility. You have a NoPreferenceStore implementation, but the preference store appears to not be an optional part of the configuration or function argument most places it is used. Will this change affect our end users or only internal implementations like sdm-core and sdm-local?

@cdupuis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

I think I can improve on backwards compatibility here. Two things that are of concern:

  • make preferenceFactory optional on the config options
  • chooseAndSetGoals should not make passing over the PreferenceStoreFactory required. It can fall back to the NoPreferenceStore implementation which should be fine. If they want to use preferences from within PushTests, they have to pass the proper store. But this would be backwards compatible.

WDTY @ddgenome?

@ddgenome

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

If it affects users, I think we should try to make it backward compatible. If it does not affect users, I think we should make the implementation as usable as possible.

@cdupuis cdupuis force-pushed the pref-store branch from b16727c to 502c824 Jan 15, 2019

@cdupuis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Latest push addresses this now. Thanks @ddgenome

@ddgenome
Copy link
Member

left a comment

LGTM, one minor comment.

Show resolved Hide resolved lib/api/context/SdmContext.ts Outdated

@cdupuis cdupuis force-pushed the pref-store branch from 502c824 to 816112c Jan 15, 2019

@cdupuis cdupuis removed the request for review from johnsonr Jan 15, 2019

@cdupuis cdupuis merged commit 38666a0 into master Jan 15, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
sdm/atomist/atomist-sdm Atomist Software Delivery Machine goals: all succeeded
Details

@cdupuis cdupuis deleted the pref-store branch Jan 15, 2019

atomist-bot added a commit that referenced this pull request Jan 15, 2019

Changelog: #658 to added
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.