Skip to content
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

Language for grouping methods #95

Merged
merged 10 commits into from Mar 17, 2017
Merged

Language for grouping methods #95

merged 10 commits into from Mar 17, 2017

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Mar 15, 2017

Since I put some group-specific logic inside Printer (see "When a group gets printed, output produced by future calls...") should we have a new log level "group" for this to be a little more detectable and rigorous? This would be used inside group() and groupCollapsed().

I think the language describing a group is both helpful and vague enough to allow browsers to implement this one way, and Node another (possibly even with prefixes since previously outputted data typically is non-interactive) while still being compliant. Thoughts?

Addresses #94

@domfarolino domfarolino self-assigned this Mar 15, 2017
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I think this would be much more explicit if there was a pointer to the current group and its parent group or similar, which were explicitly modified by group/groupCollapsed/clear/groupEnd. Maybe a group stack even, although I guess it's equivalent.

Then Printer could talk about how it prints things in the context of the current group.

WDYT?

index.bs Outdated
@@ -239,7 +239,7 @@ its \[[Prototype]] an empty object, created as if by ObjectCreate(%ObjectPrototy

<h4 id="clear" oldids="dom-console-clear" method for="console">clear()</h4>

If possible for the environment, clear the console. Otherwise, do nothing.
End all open groups, and if possible for the environment, clear the console. Otherwise, do nothing.
Copy link
Member

Choose a reason for hiding this comment

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

<a>groups</a> I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice. I didn't realize <a>groups</a> would behave like <a>group</a>

index.bs Outdated
@@ -287,12 +287,30 @@ Perform Logger("warn", <var>data</var>).

<h3 id="grouping">Grouping methods</h3>

A <dfn>group</dfn> is an implementation-specific, potentially-interactive view for output produced by calls to Printer, with one further level of indentation than its parent. A group may host output produced by calls to Printer until it is ended with <a>groupEnd()</a>, and only one group will host output produced by calls to Printer at a time.
Copy link
Member

Choose a reason for hiding this comment

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

<a>groupEnd()</a> should be {{console/groupEnd()}} to get the right formatting.

index.bs Outdated
<emu-alg>
1. Let _group_ be a new <a>group</a>.
1. Let _groupLabel_ be the result of Formatter(data), and incorporate _groupLabel_ as a label for _group_.
1. Optionally if the environment supports interactive groups, _group_ should be expanded by default.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: comma after "Optionally"

index.bs Outdated
@@ -143,7 +143,7 @@ The following is an informative summary of the format specifiers processed by th

The printer operation is implementation-defined. It accepts a log level indicating severity, and a List of arguments to print (which are either JavaScript objects, of any type, or are implementation-specific representations of printable things such as a stack trace, or output produced by the <code>%o</code> and <code>%O</code> specifiers). How the implementation prints <var>args</var> is up to the implementation, but implementations should separate the objects by a space or something similar, as that has become a developer expectation.
Copy link
Member

Choose a reason for hiding this comment

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

I think the list of things that arguments can contain (in parenthesis) needs to be expanded to include the group concept.

@domfarolino
Copy link
Member Author

Ah I almost went with a group stack but didn't know if it would be a bit much! I'll put something together referring to a group stack. Should this "global" group stack be first introduced in the text defining what a group is, or do you think it would be better somewhere in the top "abstract" area?

@domenic
Copy link
Member

domenic commented Mar 16, 2017

Hmm interesting question. I guess I might make it something associated with each console namespace object?

@domfarolino
Copy link
Member Author

domfarolino commented Mar 16, 2017

associated with each console namespace object?

Sorry, could you elaborate a bit? Are you saying it can be written in IDL under the console namespace or am I missing the idea?

In general I guess I'm not really sure how to write something that is properly "global" to the other operations. I naturally lean toward putting it near the abstract operations area because things like Printer() and Logger() are exposed everywhere without being expressed in IDL which seems convenient for private/internal implementation book-keeping.

@domenic
Copy link
Member

domenic commented Mar 16, 2017

Sorry, could you elaborate a bit? Are you saying it can be written in IDL under the console namespace or am I missing the idea?

Oh, no, I meant just something like "each console namespace object has an associated group stack, which is a stack (initially empty)."

You're right that Printer and Logger are kind of global. Maybe they should take the console object as a parameter. After all if you were implementing this you'd need some way to make sure that Printer() sends things to the right instance of developer tools.

Alternately you can just say something like "the appropriate group stack" and keep things vague. That seems fine too IMO.

@domfarolino
Copy link
Member Author

For simplicity I opted to use "appropriate group stack" instead of modify all the abstract ops to take in an instance of the correct console namespace object. Perhaps this should be looked into further at some point.

If the above changes are good, I guess the only thing left is recommending that the default group label not be an empty string if ...data is empty.

index.bs Outdated
1. Let _groupLabel_ be the result of Formatter(data), and incorporate _groupLabel_ as a label for _group_.
1. Optionally, if the environment supports interactive groups, _group_ should be expanded by default.
1. Perform Printer("log", «_group_»).
1. Push _group_ onto the appropriate <a>group stack</a>
Copy link
Member

Choose a reason for hiding this comment

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

Missing period. Also you can do <a>Push</a> (maybe <a for="stack">Push</a> will be necessary). You can feel free to not link to such basic infra terms if you think it's silly though; that's up to editor discretion.

index.bs Outdated
1. Let _groupLabel_ be the result of Formatter(data), and incorporate _groupLabel_ as a label for _group_.
1. Optionally, if the environment supports interactive groups, _group_ should be collapsed by default.
1. Perform Printer("log", «_group_»).
1. Push _group_ onto the appropriate <a>group stack</a>
Copy link
Member

Choose a reason for hiding this comment

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

Same as above on both counts

index.bs Outdated
<h4 id="groupend" oldids="dom-console-groupend" method for="console">groupEnd()</h4>

Pop the last <a>group</a> from the <a>group stack</a>.
Copy link
Member

Choose a reason for hiding this comment

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

Can optionally link to pop

Copy link
Member Author

Choose a reason for hiding this comment

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

I could not get Push/Pop to link to infra without spec="infra" (found after skimming bikeshed docs). This seems like overkill though; is this normal especially since Stack was picked up so easily?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that is an infra bug; the terms are not exported for some reason. Will fix, although it'll be ~24 hours before it gets reindexed afterward, so no need to block this PR on it.

Copy link
Member

Choose a reason for hiding this comment

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

index.bs Outdated
@@ -239,7 +239,7 @@ its \[[Prototype]] an empty object, created as if by ObjectCreate(%ObjectPrototy

<h4 id="clear" oldids="dom-console-clear" method for="console">clear()</h4>

If possible for the environment, clear the console. Otherwise, do nothing.
End all open <a>groups</a>, and if possible for the environment, clear the console. Otherwise, do nothing.
Copy link
Member

Choose a reason for hiding this comment

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

"End all open groups" should probably be "empty the appropriate group stack" right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep good call

domenic added a commit to whatwg/infra that referenced this pull request Mar 16, 2017
Also, export some list-adjacent terms.

Both changes will be useful in whatwg/console#95.
@domfarolino
Copy link
Member Author

Thanks!

@domfarolino domfarolino merged commit a44303a into master Mar 17, 2017
@domfarolino domfarolino deleted the grouping-methods branch March 17, 2017 15:55
domenic added a commit to whatwg/infra that referenced this pull request Mar 17, 2017
Also, export some list-adjacent terms.

Both changes will be useful in whatwg/console#95.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants