Skip to content

Add support for SPRING_QUIET environment variable. #674

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

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

thewoolleyman
Copy link
Contributor

@thewoolleyman thewoolleyman commented May 2, 2022

Adds support for enabling quiet mode via SPRING_QUIET environment variable.

This is useful in situations where you don't want to put Spring.quiet = true in ~/.spring.rb or your app's config/spring.rb, but want to enable quiet mode on a case-by-case basis.

See more details in the doc updates in this PR.

Closes #467

@thewoolleyman
Copy link
Contributor Author

I just noticed that @bobf already had a PR to implement this in #577

However, I'm leaving this one open, because:

  1. It has more extensive docs
  2. It short-circuits in the config reader method via ||= instead of ||, allowing it to be set back to false even after spring starts, if this is necessary.

If this PR is merged, #577 can be closed as a duplicate.

@thewoolleyman
Copy link
Contributor Author

@rafaelfranca , could we get please some attention on this one to get it merged? It will close out the following:

Thanks! 🙏

@bobf
Copy link

bobf commented May 2, 2022

Glad to see some movement on this. Please feel free to close my PR once this one is closed, or if this one ends up getting stalled somehow then I'm happy to merge the changes into my PR and make any adjustments needed.

@thewoolleyman
Copy link
Contributor Author

Hi @tenderlove 👋! any chance you or some other committer could help get some attention to getting this merged? Thanks!

@@ -52,6 +53,10 @@ def project_root_path
@project_root_path ||= find_project_root(Pathname.new(File.expand_path(Dir.pwd)))
end

def quiet
@quiet ||= ENV.key?('SPRING_QUIET')
Copy link
Contributor

Choose a reason for hiding this comment

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

It short-circuits in the config reader method via ||= instead of ||, allowing it to be set back to false even after spring starts, if this is necessary.

Correct me if I'm wrong, but at first glance it looks like that it won't be possible to set it back to false after setting it to true once, like:

class DummySpring
  class << self
    attr_writer :quiet

    def quiet
      @quiet ||= ENV.key?('SPRING_QUIET') 
    end
  end
end

ENV["SPRING_QUIET"] = "lol"
DummySpring.quiet # => true
DummySpring.quiet = false
DummySpring.quiet # => still true because `ENV.key?('SPRING_QUIET') ` gets checked again as long as `@quiet` is falsey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nvasilevski That's a good point. You could set it back to false via the attr_writer, but not via the ENV var.

But I guess I assumed that you wouldn't try to use the ENV var to change it while an app is executing - if you want to do that, just use the attr_writer.

In other words, the ENV var is only needed when you don't have access to modify the code or config at all, but still want to turn quiet mode on,

This is also explained by the new documentation:

You can also set the initial state of the quiet configuration option to true
by setting the SPRING_QUIET environment variable before executing Spring.

Note it says "initial state" and "...before executing Spring".

So, I guess I think this is OK as is, especially given the documentation.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

You could set it back to false via the attr_writer, but not via the ENV var.

In my example I'm showing that it's not possible to reset it using writer after setting it once to true because of how ||= works. If we want to support that, reader will need to be implemented as:

def quiet
    return @quiet if defined(@quiet)

    @quiet = ENV.key?('SPRING_QUIET')
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my example I'm showing that it's not possible to reset it using writer after setting it once to true because of how ||= works.

Ah, gotcha, you're right.

So ideally we can come up with a case where it can be changed from true to false or vice versa, via either the attr_writer or the ENV var.

I'll try to come up with a change that does that and then ping you to review it.

Thanks for your help and attention!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I got busy/lazy and never returned to finish up this thread.

But I see this got merged now, which is great!

This thread describes a minor bug which should be fixed eventually, so I'll keep it on my TODO list - near the bottom, which realistically means "probably never", but hope springs eternal ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, there was an additional commit on top of this PR to remove the memoization and make ENV variable to take precedence over a falsey @quiet value

1908710

@rafaelfranca rafaelfranca merged commit 67492f2 into rails:main Sep 20, 2022
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.

Spring stderr output causing warnings in other apps
4 participants