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 types that help you specify what your Configuration is #679

Merged
merged 10 commits into from Feb 27, 2019

Conversation

Projects
None yet
5 participants
@jessitron
Copy link
Contributor

commented Feb 26, 2019

end goal looks like this: atomist/atomist-sdm#103

jessitron and others added some commits Feb 26, 2019

Autofix: TypeScript header
[atomist:generated] [atomist:autofix=typescript_header]

@jessitron jessitron changed the title Nortissej/add options Add types that help you specify what your Configuration is Feb 26, 2019

@jessitron jessitron requested a review from cdupuis Feb 26, 2019

@jessitron jessitron marked this pull request as ready for review Feb 26, 2019

@jessitron jessitron force-pushed the nortissej/addOptions branch from 3c87aad to 0556cf7 Feb 26, 2019

@jessitron jessitron force-pushed the nortissej/addOptions branch from 0556cf7 to 9e2109a Feb 26, 2019

@lievendoclo
Copy link
Contributor

left a comment

LGTM

@ddgenome
Copy link
Member

left a comment

A couple questions.

* limitations under the License.
*/

export interface AdditionalSdmConfiguration<ConfigurationType> {

This comment has been minimized.

Copy link
@ddgenome

ddgenome Feb 27, 2019

Member

Doesn't an equivalent type already exist?

This comment has been minimized.

Copy link
@ddgenome

ddgenome Feb 27, 2019

Member

I was thinking of this

export interface SoftwareDeliveryMachineConfiguration extends Configuration {
which seems different but related maybe.

This comment has been minimized.

Copy link
@ddgenome

ddgenome Feb 27, 2019

Member

Is ConfigurationType just a type parameter?

This comment has been minimized.

Copy link
@jessitron

jessitron Feb 27, 2019

Author Contributor

yes, it is just a type parameter. Christian specifically suggested a wrapper type to put it inside the sdm property

This comment has been minimized.

Copy link
@cdupuis

cdupuis Feb 27, 2019

Contributor

I'm not too happy with AdditionalSdmConfiguration: all the other config and options types call it SoftwareDeliveryMachine instead of Sdm. But I also don't have any other suggestion.

export interface SdmCacheConfiguration {
cache: {
enabled: boolean;
path: string;

This comment has been minimized.

Copy link
@ddgenome

ddgenome Feb 27, 2019

Member

Are neither of these optional?

This comment has been minimized.

Copy link
@jessitron

jessitron Feb 27, 2019

Author Contributor

hmm true, path is optional. If you supply the cache configuration then you should supply enabled.

@cdupuis
Copy link
Contributor

left a comment

I do wonder if we need a new type for this or could just parameterise SoftwareDeliveryMachineConfiguration with SoftwareDeliveryMachineConfiguration<ConfigurationType>. Then people could just do:

const configuration: SoftwareDeliveryMachineConfiguration<CacheConfiguration> & SoftwareDeliveryMachineConfiguration<K8sConfiguration> = {}

And then I wonder if we can make that something like:

const configuration: SoftwareDeliveryMachineConfiguration<CacheConfiguration & K8sConfiguration> = {}

On that note, I would suggest that we make all properties of SoftwareDeliveryMachineConfiguration optional because they are being defaulted anyway. This should make using that type a lot easier.

* limitations under the License.
*/

export interface AdditionalSdmConfiguration<ConfigurationType> {

This comment has been minimized.

Copy link
@cdupuis

cdupuis Feb 27, 2019

Contributor

I'm not too happy with AdditionalSdmConfiguration: all the other config and options types call it SoftwareDeliveryMachine instead of Sdm. But I also don't have any other suggestion.

@cdupuis

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

I should clarify that I'm talking about the following:

/**
 * Configuration that takes SoftwareDeliveryMachineOptions inside the sdm key.
 */
export interface SoftwareDeliveryMachineConfiguration<ConfigurationOptions = any> extends Configuration {
    sdm: SoftwareDeliveryMachineOptions & AnyOptions & ConfigurationOptions;
}

and then:

const configuration: SoftwareDeliveryMachineConfiguration<CacheConfiguration> = {};
@cdupuis

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@jessitron this looks good, doesn't it? And it should be backwards compatible.

We can even do:

const configuration: SoftwareDeliveryMachineConfiguration<CacheConfiguration & K8sConfiguration> = {};
@jessitron

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

you're always gonna need at least an sdm property right?

... do you want the CacheConfiguration to make the sdm.cache property optional? ... or if you declare that you have CacheConfiguration, should you also have a sdm.cache element?

@jessitron jessitron merged commit 71a5a74 into master Feb 27, 2019

2 checks passed

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

@jessitron jessitron deleted the nortissej/addOptions branch Feb 27, 2019

atomist-bot added a commit that referenced this pull request Feb 27, 2019

Changelog: #679 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.