Make append(DSL) ignore value which inserted before

[riseshia]

Summary

I propose append dsl makes array to be unique.

This proposal makes below situations to be avoidable:

# in capistrano-puma gem
append :rbenv_map_bins, 'pumactl'

# deploy.rb
append :rbenv_map_bins, 'pumactl'

$HOME/.rbenv/bin/rbenv exec $HOME/.rbenv/bin/rbenv exec bundle exec bundle exec pumactl -S /home/deploy/application/shared/tmp/pids/puma.state -F ...

Please notice that command has duplicated rbenv exec.

I think these kind of problem don't occur frequently but hard to find. and also, until append introduced, there is gems used set accidently (I believe).
This problem may occur when a user tries to update gems which contains patch replacing set to append.

One more, this makes plugin developer do not care about the other plugins, especially duplicated adding configuration variable.

Short checklist

• Did you run bundle exec rubocop -a to fix linter issues?
• If relevant, did you create a test?
• Did you confirm that the RSpec tests pass?
• If you are fixing a bug or introducing a new feature, did you add a CHANGELOG entry?
  • I will do when this proposal confirmed. XD

Thanks to read this. Have a nice day :)

[capistrano-bot]

	1 Warning
⚠️	Please update CHANGELOG.md with a description of your changes. If this PR is not a user-facing change (e.g. just refactoring), you can disregard this.

Here's an example of a CHANGELOG.md entry (place it immediately under the * Your contribution here! line):

* [#1936](https://github.com/capistrano/capistrano/pull/1936): Make append(DSL) ignore value which inserted before - [@riseshia](https://github.com/riseshia)

Generated by 🚫 Danger

[will-in-wi]

Interesting idea. I can't think of a situation where I would want the array to include duplicates, but from my experience maintaining this project, the instant we make that not possible, someone else will come tell us about their workflow, which we just broke…

[will-in-wi]

I wonder whether we simply need to be more specific about how #append works. Right now, it looks like:

set(key, Array(fetch(key)).concat(values))

Which coerces into an array, and then adds a value. We may just want to only operate on arrays (or array-like things, like Set) and fail if it isn't an array. Then we could handle Array and Set and change capistrano/rbenv to use Set instead since it doesn't really make sense to duplicate binary names in this case.

Other maintainers: thoughts?

[will-in-wi]

Another fix is to make capistrano/rbenv more resilient to this.

This https://github.com/capistrano/rbenv/blob/master/lib/capistrano/tasks/rbenv.rake#L22 could be changed to uniq the array before iterating over it.

Or maybe go further back and make SSHKit.config.command_map not allow duplicates.

[mattbrictson]

Hmm. I don't think we should change Capistrano's behavior in this case. The method is called append, and it would be surprising/misleading if behind the scenes it is actually doing something different and more complicated.

I think the responsibility lies in capistrano/rbenv to call uniq.

[mattbrictson]

Alternatively we could define a new DSL method called add that follows set semantics. (Not saying that's a good idea, but putting it out there.)

[riseshia]

Thanks to quick response. :)

The method is called append, and it would be surprising/misleading if behind the scenes it is actually doing something different and more complicated. I think the responsibility lies in capistrano/rbenv to call uniq.

I agree with proposed behavior may make misleading, however, for example, not only capistrano/rbenv, could we say to the maintainers of whole plugin which want to use append like 'ok, append don't grant uniqueness, so you check uniqueness when use append variable if you need'? Instead of this, It's better to provide something which grant uniqueness I think, also it helps many plugin works more reliable from another plugin or user's mistake.

I don't say this proposal accepted, but we may need some way to keep uniqueness of enumerable.

@will-in-wi 's ideas seems good to me, but It seems to be better that define new dsl for this. Explicit way makes better understanding how to use capistrano, but also makes it easy where to dig if we have problem related this.

[will-in-wi]

It seems to me that there is agreement that we shouldn't change #append, and if we're going to do anything we should create an #add method. I'm closing this PR.

Thank you for submitting this contribution and working to make Capistrano better!