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

rm and stopMany methods have an inconsistent API #223

Open
johnytiago opened this issue Jan 24, 2023 · 10 comments
Open

rm and stopMany methods have an inconsistent API #223

johnytiago opened this issue Jan 24, 2023 · 10 comments

Comments

@johnytiago
Copy link
Contributor

The signatures of the rm and stopMany methods is inconsistent with other methods that accept multiple services.
For all the other methods that accept multiple services the signature is services first, options second:

 (services: string[], options?: IDockerComposeOptions | undefined) => Promise<IDockerComposeResult>;

Yet, for rm and stopMany the signature is different:

(options?: IDockerComposeOptions | undefined, ...services: string[]) => Promise<IDockerComposeResult>

I know that changing this signature is likely to result in a breaking change, but would you be willing to accept a PR to fix this?

@AlexZeitler
Copy link
Contributor

We're still below 1.0.0 release, so everything is still subject to changes.

@Steveb-p any worries about this?

@Steveb-p
Copy link
Contributor

This sounds good actually. The function signature looks a lot better the way @johnytiago suggests.

Of course I'm worried about a breaking change that this would introduce. Ideally we would have a way of dealing with it, maybe by detecting that we're dealing with an old invocation (maybe by checking if the first argument is an array?) and swapping the arguments as necessary? 🤔

@AlexZeitler
Copy link
Contributor

A silent upgrade for function calls would be nice. Not sure if we allow services to be a string as well.

@johnytiago
Copy link
Contributor Author

We could likely come up with a way to make this backwards compatible, but we'd still be introducing a special signature to this two functions only. The inconsistency we'd be introducing is better than the one we had before, though.

Regarding versioning and breaking changes, you could postpone removing the old signature for only when you feel like you have enough to go onto v1.0.0.

@AlexZeitler
Copy link
Contributor

Maybe we can mark the old signature as deprecated?

@Steveb-p
Copy link
Contributor

The easiest way of handling it would indeed be adding new functions, instead of trying to shove into the old ones. It would also help with types (or rather, we would not need to do any shenanigans with function declaration, as we're strictly declaring acceptable types currently, and we would probably need to mark lines that perform invalid type checks as ignored).

@AlexZeitler
Copy link
Contributor

Just call them rm2 and stopMany2? Might be misleading regarding compose v2 maybe? However we don't have no v2 stuff there.

@AlexZeitler
Copy link
Contributor

I would flag the old ones deprecated anyway so people get aware of it.

@johnytiago
Copy link
Contributor Author

I don't think adding new functions is going to fix the problem either.
1 - the existing inconsistency between the many other *Many functions will still exist
2 - you'll be introducing yet another one. A different naming convention that you'd expect to act the same.

Plus, I don't think this is a problem enough that people need this feature out to resolve their problems. It's just unpleasant quirk one can easily fix on their end.

I'd say the best approach, that respects semantic versioning, is to maintain both signatures temporarily until the later gets deprecated on a major bump.

Something like this:

function stopMany( options?: IDockerComposeOptions, ...services: string[]): Promise<IDockerComposeResult>;
function stopMany( services: string[], options?: IDockerComposeOptions | undefined):  Promise<IDockerComposeResult>;
function stopMany( services: unknown, options: unknown, ...rest) {
    if (Array.isArray(services)) {
        return execCompose('stop', services, options)
    }

    const _options = services
    const _services =  [ options, ...rest ]
    return execCompose('stop', _services, _options)
}

@AlexZeitler
Copy link
Contributor

I think this should work.

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

No branches or pull requests

3 participants