Added Deprecate Property to Watch Flag (Issue #3256) #3440
Conversation
abdullahzameek
changed the title
@nicks So, for --hud=true, it just shows up like this: You see it momentarily, and the terminal window switches to the tilt build progress window. It doesn't show up in the actual hud logs, but once you exit the hud, it just shows up below "tilt up" on the terminal |
|
oof, that's frustrating. if you want to merge this pr as-is and iterate on it, that's fine. if you look at how What if we checked if that flag was set below that line, and logged the message there? |
I'm reading through it, I'll try to fix it and then merge it all in one go! |
|
@nicks Managed to put the warning into the logs!
If you view it from the TUI though, you see the message being printed twice - once called by flag.go file at https://github.com/tilt-dev/tilt/blob/master/vendor/github.com/spf13/pflag/flag.go#L485 and then again at when the logger output is called. So, if you do --hud=false, you'd see this: I'm not really sure how I can disable the function from the flag.go from printing the warning, other than by modifying the code from there itself, but that seems like a bad idea since its an external library... |
internal/cli/up.go
Outdated
| cmd.Flag("watch").Deprecated = "it will be removed in future releases." | ||
| //if a deprecation message exists, then watch was explicitly set by the user, so mark the watchFlagExplicitlySet var as true for | ||
| //logging the message on the web UI and TUI | ||
| if len(cmd.Flag("watch").Deprecated) > 0 { |
There was a problem hiding this comment.
i don't think this is doing what you expect...this will always be true, because you just set it above.
we probably shouldn't use the Deprecated field at all, because we don't want the flag library to do its own printing?
There was a problem hiding this comment.
Ohh right, I assumed that the Deprecated property is only invoked when the user explicitly uses the flag in the command line, and that it would just be an empty string otherwise. Turns out that is not the case, my bad.
Yeah, I think I'll look out for another way to do this because of the flag library printing.
(Also, apologies for the late reply, it was getting late yesterday and I was a bit tired)
There was a problem hiding this comment.
I think it's good now! I got too hung up on utilizing the Deprecated field (since I thought that was the best way to get about it) but turns out, it was just a matter of adding another line under the existing logic:
8ee678d#diff-97a466a66696420cf5da35a819c2d535L106-L112
Its printing the deprecation warning on the logs both on the TUI and the Web UI properly now (and doesn't print the message when the flag isn't present)
| @@ -98,6 +101,7 @@ local resources--i.e. those using serve_cmd--are terminated when you exit Tilt. | |||
|
|
|||
| cmd.PreRun = func(cmd *cobra.Command, args []string) { | |||
| c.hudFlagExplicitlySet = cmd.Flag("hud").Changed | |||
| c.watchFlagExplicitlySet = cmd.Flag("watch").Changed | |||
internal/cli/up.go
Outdated
| @@ -136,6 +140,11 @@ func (c *upCmd) run(ctx context.Context, args []string) error { | |||
|
|
|||
| logOutput(fmt.Sprintf("Starting Tilt (%s)…", buildStamp())) | |||
|
|
|||
| //if --watch was set, warn user about deprecation | |||
| if c.watchFlagExplicitlySet { | |||
| logOutput("Flag --watch has been deprecated, it will be removed in future releases.") | |||
There was a problem hiding this comment.
fyi, i think you can also use logger.Get(ctx).Warnf(...) if you want it to show up as a warning in the UI
There was a problem hiding this comment.
Warnf() seems to be doing something interesting - it prints the warning fine on the TUI, but the keyword "warning" doesn't show up on the Web UI for some reason.
There was a problem hiding this comment.
yep! it has the yellow UI bar in the web, and skips the prefix
Fixes #3256
I've added the deprecate property to the watch flag. The result of doing so is as follows:
When "--watch" is used:
When there's no "--watch" included:
