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

Remove Config.generate_command and update tests #259

Merged
merged 10 commits into from
Mar 17, 2021

Conversation

mutecipher
Copy link
Contributor

@mutecipher mutecipher commented Mar 15, 2021

Closes: #254

Motivation

Dynamic headers cause false positives and unnecessary changes when regenerating RBIs. Hard coded the commands for generation and removed deprecated the generate_command from Tapioca::Config as it is no longer necessary.

Implementation

Updated tests and implementations to use hardcoded tapioca <command> <constant>. Also, added binstub for tapioca.

Tests

Added assertion for the presence of bin/tapioca.

@mutecipher mutecipher self-assigned this Mar 15, 2021
@mutecipher mutecipher requested a review from a team March 15, 2021 20:30
Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

Can we make it the default value of the option instead of removing the option?

@mutecipher
Copy link
Contributor Author

Can we make it the default value of the option instead of removing the option?

@Morriar There's some difficulty with that. Mainly because there is no default value for generating RBIs. Depending on what we generate different commands are used (i.e. Gems created with tapioca sync, and DSLs with tapioca dsl).

Also, feels kind of like a weird/non-standard flag to me. As you're basically telling the program to lie about what it's accomplished.

I'm open to being convinced otherwise as to where it's useful.

@Morriar
Copy link
Collaborator

Morriar commented Mar 15, 2021

There's some difficulty with that.

Can't we reach the same result with something like:

    class_option :generate_command,
                  aliases: ["--cmd", "-c"],
                  banner: "command",
                  desc: "The command to run to regenerate RBI files",
                  default: "tapioca"

then:

say("RBI files are out-of-date, please run `#{config.command} dsl` to update.")

?

@mutecipher
Copy link
Contributor Author

mutecipher commented Mar 16, 2021

There's some difficulty with that.

Can't we reach the same result with something like:

    class_option :generate_command,
                  aliases: ["--cmd", "-c"],
                  banner: "command",
                  desc: "The command to run to regenerate RBI files",
                  default: "tapioca"

then:

say("RBI files are out-of-date, please run `#{config.command} dsl` to update.")

?

That's more or less what we're trying to get rid of. Yes, you could specify the command that you want in the header, but that seems an obtuse way of accomplishing this task IMO. In this case, we'd have to specify a -C/--cmd flag anytime we pass flags into tapioca in order to generate the correct header.

@Morriar
Copy link
Collaborator

Morriar commented Mar 16, 2021

That's more or less what we're trying to get rid of. Yes, you could specify the command that you want in the header, but that seems an obtuse way of accomplishing this task IMO. In this case, we'd have to specify a -C/--cmd flag anytime we pass flags into tapioca in order to generate the correct header.

My understanding of the road map was to create a config file to store such option so we don't have to pass them each time but still have the option available.

Stating tapioca X in the header when we actually expect them to run bundle exec tapioca X or another command will come around to bite us with people committing files produced by the wrong version of tapioca.

@mutecipher
Copy link
Contributor Author

mutecipher commented Mar 16, 2021

My understanding of the road map was to create a config file to store such option so we don't have to pass them each time but still have the option available.

Stating tapioca X in the header when we actually expect them to run bundle exec tapioca X or another command will come around to bite us with people committing files produced by the wrong version of tapioca.

According to the Slack thread, Rafael suggested we always use tapioca dsl but I believe we've had discussions of a binstub but nothing concrete has come out of that yet. But as it stands, there's two different headers that could get generated tapioca dsl for DSLs and tapioca sync for Gems.

I think these static headers are a good work around for now, but I think long term I'd like to see that bundled up into a single command, tapioca sync that can modify behaviour using flags.

@mutecipher mutecipher requested a review from a team March 16, 2021 14:40
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

We would also like to remove the default for generate_command in config_builder.rb and the default_command method in the same file.

lib/tapioca/cli.rb Outdated Show resolved Hide resolved
lib/tapioca/generator.rb Outdated Show resolved Hide resolved
spec/tapioca/cli_spec.rb Outdated Show resolved Hide resolved
spec/tapioca/cli_spec.rb Outdated Show resolved Hide resolved
spec/tapioca/cli_spec.rb Outdated Show resolved Hide resolved
spec/tapioca/cli_spec.rb Outdated Show resolved Hide resolved
@paracycle
Copy link
Member

Stating tapioca X in the header when we actually expect them to run bundle exec tapioca X or another command will come around to bite us with people committing files produced by the wrong version of tapioca.

We run the same risk with people doing rails xxx as well. Hopefully, people are using our tools either with the binstubs or via bundle exec.

One thing we can do, however, is to make Tapioca create the binstub in tapioca init and then to set these commands as bin/tapioca xxx unconditionally. I think that would address @Morriar's concern.

@paracycle
Copy link
Member

The binstub creation code should be something like:

installer = Bundler::Installer.new(Bundler.root, Bundler.definition)
spec = Bundler.definition.specs.find {|s| s.name == "tapioca" }
installer.generate_bundler_executable_stubs(spec, { force: true })

lib/tapioca/generator.rb Outdated Show resolved Hide resolved
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Like @Morriar pointed out, we need to s|tapioca |bin/tapioca |g or something in file headers and commands that we show.

Also, we need the config_builder.rb changes.

lib/tapioca/cli.rb Outdated Show resolved Hide resolved
spec/tapioca/cli_spec.rb Show resolved Hide resolved
lib/tapioca/config_builder.rb Outdated Show resolved Hide resolved
lib/tapioca/config_builder.rb Outdated Show resolved Hide resolved
@mutecipher mutecipher merged commit 287f37d into master Mar 17, 2021
@mutecipher mutecipher deleted the remove-dynamic-rbi-header branch March 17, 2021 17:31
@mutecipher mutecipher temporarily deployed to production March 22, 2021 20:31 Inactive
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.

Replace the header in tapioca so that it doesn't change so often
4 participants