Only log halted pipes by default #1062

paulcsmith · 2020-03-24T01:39:07Z

But allow customizing it so that you can see all pipes being logged

paulcsmith · 2020-03-24T01:50:48Z

spec/lucky/action_callbacks_spec.cr

@@ -207,7 +207,7 @@ private def with_log
    log_formatter: RawLogFormatter
  )

-  Lucky.temp_config(logger: logger) do
+  Lucky.temp_config(logger: logger, action_pipe_log_level: Logger::Severity::INFO) do


Actually, should we just call these before/after macros callbacks? We kind of have callbacks and pipes in the code and guides so I think we should choose one.

I partly named them pipes because they work a bit more similarly to pipes in Elixir where you have to return an explicit response/continue. And also because "callbacks" is a bad word to some people. But maybe we should just call them callbacks since that may make more sense. Thoughts? @edwardloveall @jwoertink

I think pipes is fine, but I don't think I feel too strongly either way. There's also an ick factor about callbacks for me, but not so strong that I'd steer away from the word itself if it's the best word to discribe.

I like pipes because it sounds cool. Maybe that's because of callbacks having a bad taste, but we use the term "pipe" in our apps. I would like to see it a bit more standardized, and consistent though.

Like we have:

module RequireSitePipe macro included before require_site end private def require_site if current_site? continue else raise Lucky::RouteNotFoundError.new(context) end end private def current_site : SiteHelpers::Site current_site?.not_nil! end end

but just by looking at the name of the module, it doesn't immediately tell you why we added "Pipe" to it. We could have just called it RequireSite.

As mentioned in our gitter room I'll stick with pipes. I was just worried it was confusing. I'll make some changes to the code to be pipe instead of callback!

Part of: luckyframework/lucky#1062

edwardloveall · 2020-03-24T12:28:16Z

src/lucky.cr

@@ -22,6 +22,7 @@ require "./lucky/paginator/*"
 module Lucky
  Habitat.create do
    setting logger : Dexter::Logger
+    setting action_pipe_log_level : ::Logger::Severity? = nil, example: "Logger::Severity::INFO"


What does example mean here? I think I could guess but I've never seen it. 😄

example is what gets displayed in the error message when it's not set. But since this is nilable, does it throw an error?

Yeah it shows up if you forget to set it, but Jeremy is right that you'll never see it :P

I think I will leave it though since in the future I want to add a task to Habitat that will show the options available, examples, and also where the configuration is being set. It'll be pretty sick I think

edwardloveall · 2020-03-24T12:33:06Z

spec/lucky/action_callbacks_spec.cr

@@ -207,7 +207,7 @@ private def with_log
    log_formatter: RawLogFormatter
  )

-  Lucky.temp_config(logger: logger) do
+  Lucky.temp_config(logger: logger, action_pipe_log_level: Logger::Severity::INFO) do


I think pipes is fine, but I don't think I feel too strongly either way. There's also an ick factor about callbacks for me, but not so strong that I'd steer away from the word itself if it's the best word to discribe.

jwoertink

👍

jwoertink · 2020-03-24T14:10:57Z

spec/lucky/action_callbacks_spec.cr

@@ -207,7 +207,7 @@ private def with_log
    log_formatter: RawLogFormatter
  )

-  Lucky.temp_config(logger: logger) do
+  Lucky.temp_config(logger: logger, action_pipe_log_level: Logger::Severity::INFO) do


I like pipes because it sounds cool. Maybe that's because of callbacks having a bad taste, but we use the term "pipe" in our apps. I would like to see it a bit more standardized, and consistent though.

Like we have:

module RequireSitePipe macro included before require_site end private def require_site if current_site? continue else raise Lucky::RouteNotFoundError.new(context) end end private def current_site : SiteHelpers::Site current_site?.not_nil! end end

but just by looking at the name of the module, it doesn't immediately tell you why we added "Pipe" to it. We could have just called it RequireSite.

jwoertink · 2020-03-24T14:12:35Z

src/lucky.cr

@@ -22,6 +22,7 @@ require "./lucky/paginator/*"
 module Lucky
  Habitat.create do
    setting logger : Dexter::Logger
+    setting action_pipe_log_level : ::Logger::Severity? = nil, example: "Logger::Severity::INFO"


example is what gets displayed in the error message when it's not set. But since this is nilable, does it throw an error?

jwoertink · 2020-03-24T14:14:08Z

src/lucky/action_callbacks.cr

@@ -127,10 +127,10 @@ module Lucky::ActionCallbacks
      #   end

      if callback_result.is_a?(Lucky::Response)
-        Lucky::ActionCallbacks.log_stopped_callback(context,"{{ callback_method.id }}")
+        Lucky::ActionCallbacks.log_stopped_callback("{{ callback_method.id }}")


Yeah, I see what you mean. This should be Lucky::ActionPipes.log_stopped_pipe, or we go full on callback everywhere. I do like "pipe", but maybe we don't fight it since typing callback just seems to be what you want to do first?

But allow customizing it so that you can see all pipes being logged Closes #1054

paulcsmith

Thanks for the reviews

paulcsmith · 2020-03-24T16:29:48Z

spec/lucky/action_callbacks_spec.cr

@@ -207,7 +207,7 @@ private def with_log
    log_formatter: RawLogFormatter
  )

-  Lucky.temp_config(logger: logger) do
+  Lucky.temp_config(logger: logger, action_pipe_log_level: Logger::Severity::INFO) do


As mentioned in our gitter room I'll stick with pipes. I was just worried it was confusing. I'll make some changes to the code to be pipe instead of callback!

paulcsmith · 2020-03-24T16:30:34Z

src/lucky.cr

@@ -22,6 +22,7 @@ require "./lucky/paginator/*"
 module Lucky
  Habitat.create do
    setting logger : Dexter::Logger
+    setting action_pipe_log_level : ::Logger::Severity? = nil, example: "Logger::Severity::INFO"


Yeah it shows up if you forget to set it, but Jeremy is right that you'll never see it :P

I think I will leave it though since in the future I want to add a task to Habitat that will show the options available, examples, and also where the configuration is being set. It'll be pretty sick I think

Part of: luckyframework/lucky#1062

jkthorne

☕️

paulcsmith · 2020-03-24T20:07:57Z

It is passing but not reporting back to GitHub so I'll merge https://travis-ci.org/github/luckyframework/lucky

Part of: luckyframework/lucky#1062

edwardloveall · 2020-03-24T20:34:33Z

We had that problem on a different project also where circle wasn't reporting to GH. Maybe it's the same problem? hotline-webring/hotline-webring#273

Part of: luckyframework/lucky#1062

paulcsmith mentioned this pull request Mar 24, 2020

Only add TestBackdoor in test env luckyframework/lucky_cli#457

Closed

paulcsmith commented Mar 24, 2020

View reviewed changes

paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Mar 24, 2020

Add default with comments for pipe log

e25dff6

Part of: luckyframework/lucky#1062

paulcsmith mentioned this pull request Mar 24, 2020

Add default with comments for pipe log luckyframework/lucky_cli#485

Merged

paulcsmith requested review from edwardloveall and jwoertink March 24, 2020 01:56

edwardloveall approved these changes Mar 24, 2020

View reviewed changes

jwoertink approved these changes Mar 24, 2020

View reviewed changes

Only log halted pipes by default

d4c9e2c

But allow customizing it so that you can see all pipes being logged Closes #1054

paulcsmith commented Mar 24, 2020

View reviewed changes

paulcsmith force-pushed the pcs/1054-only-log-halt-by-default branch 4 times, most recently from b3a7e6b to bf1c82d Compare March 24, 2020 17:15

paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Mar 24, 2020

Add default with comments for pipe log

8befbc0

Part of: luckyframework/lucky#1062

Refactor code from callbacks -> pipes

1817128

paulcsmith force-pushed the pcs/1054-only-log-halt-by-default branch from bf1c82d to 1817128 Compare March 24, 2020 19:51

jkthorne approved these changes Mar 24, 2020

View reviewed changes

paulcsmith merged commit 5d207b2 into master Mar 24, 2020

paulcsmith deleted the pcs/1054-only-log-halt-by-default branch March 24, 2020 20:08

paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Mar 24, 2020

Add default with comments for pipe log

9063911

Part of: luckyframework/lucky#1062

paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Mar 24, 2020

Add default with comments for pipe log

0b2f8bf

Part of: luckyframework/lucky#1062

paulcsmith added a commit to luckyframework/lucky_cli that referenced this pull request Apr 7, 2020

Add default with comments for pipe log

234eab0

Part of: luckyframework/lucky#1062

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only log halted pipes by default #1062

Only log halted pipes by default #1062

paulcsmith commented Mar 24, 2020

paulcsmith Mar 24, 2020

edwardloveall Mar 24, 2020

jwoertink Mar 24, 2020

paulcsmith Mar 24, 2020

edwardloveall Mar 24, 2020

jwoertink Mar 24, 2020

paulcsmith Mar 24, 2020

edwardloveall Mar 24, 2020

jwoertink left a comment

jwoertink Mar 24, 2020

jwoertink Mar 24, 2020

jwoertink Mar 24, 2020

paulcsmith left a comment

paulcsmith Mar 24, 2020

paulcsmith Mar 24, 2020

jkthorne left a comment

paulcsmith commented Mar 24, 2020

edwardloveall commented Mar 24, 2020

Only log halted pipes by default #1062

Only log halted pipes by default #1062

Conversation

paulcsmith commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwoertink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkthorne left a comment

Choose a reason for hiding this comment

paulcsmith commented Mar 24, 2020

edwardloveall commented Mar 24, 2020