Skip to content

Comments

Improve install experience#510

Merged
leastbad merged 21 commits intostimulusreflex:masterfrom
leastbad:fix_sanity
Jul 19, 2021
Merged

Improve install experience#510
leastbad merged 21 commits intostimulusreflex:masterfrom
leastbad:fix_sanity

Conversation

@leastbad
Copy link
Contributor

Type of PR (feature, enhancement, bug fix, etc.)

Bug fix

Description

It doesn't seem like the sanity check modules receive anything in the ARGV, and since it's basically impossible to obtain the CLI command used to run the rake task, I suspect that the called_by_generate_config? method isn't doing anything.

I came up with an admittedly hacky but seemingly effective way to determine if the sanity check is being run inside of rake:

    def called_by_rake?
      caller.find { |c| c.include?("/gems/rake-") }
    end

I have been able to verify that installation works great on a fresh Rails 6.1 project.

I also took the opportunity to freshen up the copy offered by the warning messages. I found it a bit hard to read.

Fixes #508

Install task needs to be rock solid.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@leastbad leastbad added bug Something isn't working ruby Pull requests that update Ruby code infrastructure labels May 23, 2021
@leastbad leastbad added this to the 3.5 milestone May 23, 2021
@leastbad leastbad requested a review from hopsoft May 23, 2021 09:17
@leastbad leastbad self-assigned this May 23, 2021
@leastbad leastbad requested a review from marcoroth May 23, 2021 09:20
Copy link
Member

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

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

Nice! I really like the overall improvements done here 👍🏼

@leastbad leastbad changed the title parse Kernel#callers for references to rake Improve install experience Jun 30, 2021
Copy link
Contributor

@KonnorRogers KonnorRogers left a comment

Choose a reason for hiding this comment

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

In my brief attempts to break this, it held up pretty well. Default urls not being set isnt currently firing. I suggested a fix. Other than that, this feels pretty solid.

@leastbad leastbad merged commit 1838690 into stimulusreflex:master Jul 19, 2021
@leastbad leastbad deleted the fix_sanity branch August 16, 2021 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working infrastructure ruby Pull requests that update Ruby code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sanity checker should not abort stimulus_reflex install task

3 participants