Skip to content

Conversation

@come-maiz
Copy link

The word Stream is confusing. It has many meanings, and it's not clear that it means more verbose than verbose. In addition to that, using a numerical value allows future runners and reporters to have a mode that's even more verbose than Stream.

Requires #71.

Leo Arias added 2 commits January 27, 2016 13:24
If the runner needs the Stream value from the config, it should not get
it from the reporter. It is better to keep a copy of the value in the
runner itself to make the communication between runner and reporter only
one-direction.
@come-maiz
Copy link
Author

I reverted the change to the RunConf type.

Choose a reason for hiding this comment

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

I would use a type here for the enumerated constant values, take a look at https://golang.org/doc/effective_go.html#constants

Copy link
Author

Choose a reason for hiding this comment

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

I used a number instead of constants because we don't have names for the different levels.
We now could use lowVerbosity and highVerbosity. But we already need three levels of verbosity for the snappy tests, and we will probably add an extra level at some point.

@fgimenez
Copy link

@ElOpio looks good, minor comment :)

@niemeyer
Copy link
Contributor

Stream means do not cache output.. Verbose means show more output than usual. This is significantly more clear than Verbose being 1 or 2. Can we please focus on actual improvements?

@niemeyer niemeyer closed this Jan 29, 2016
@come-maiz
Copy link
Author

@niemeyer that is the problem. You are using the log as a cache for the reporter, sometimes, depending on the flags. In #75 I split them, as the log should have its own configuration separate from the reporter. And if you want the log to also go to stdout, or to somewhere else, we can add that.
You say that stream means do not cache, but that's not true. The only way to use it now is to pass -vv, which means more verbose than -v.
That's why I proposed to fully get rid of Stream in the other PR you closed.
For those reasons, this is an improvement with respect to the current state.

@niemeyer
Copy link
Contributor

that is the problem. You are using the log as a cache for the reporter, sometimes, depending on the flags.

How is that a problem?

You say that stream means do not cache, but that's not true.

How is it not true?

@come-maiz
Copy link
Author

Also, it would be nice if before closing the PRs, we had a discussion about them. I'm explaining the rationale for all my changes, and why they are needed to finally implement the subunit reporter that's all I want. You are here not offering alternatives, just saying that the work I did improves in no way your project, rejecting it before I can even reply. And if that's the case, if this is no improvement at all, give me a hand and offer some suggestions.

@niemeyer
Copy link
Contributor

Also, it would be nice if before closing the PRs, we had a discussion about them.

It would be even better if we had a conversation before opening a PR.

@come-maiz
Copy link
Author

We had a conversation, and you told me to implement the interface in a correct way that could be moved to a separate package. Here it is, #76. Split in tiny progressive improvements for having a pleasant discussion on each of them.

How is it a problem that the log and the reporter are too coupled? It prevents us from writing an independent reporter. Also from writing an independent log, which would be nice too have too.

How is it not true that stream mean cache? Because "The only way to use it now is to pass -vv, which means more verbose than -v." So stream means -vv.

@niemeyer
Copy link
Contributor

We had a conversation (...)

No, we didn't Leo. This PR proposes a change we never talked about.

How is it not true that stream mean cache? Because "The only way to use it now is to pass -vv, which means more verbose than -v." So stream means -vv._

Stream and Verbose is part of the implementation and of a public API. Stream means "stream results out as soon as you get them", Verbose means "tell me more about what you are doing". This both how it is internally implemented, and exposed in the public-facing API of the package. The fact -vv sets both is irrelevant, and does not change any of those facts.

How is it a problem that the log and the reporter are too coupled?
It prevents us from writing an independent reporter.

Please leave the implementation with me Leo. We're wasting our time talking about two booleans instead of making any progress at all on the actual reporter interface.

@come-maiz
Copy link
Author

as you wish. Please give me an ETA for when we will have the subunit reporter, so I can tell the certification team when to expect it.

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.

3 participants