-
Notifications
You must be signed in to change notification settings - Fork 419
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
Add field reuse links to relevant field sets #569
Conversation
@benskelker I didn't realize The build is breaking because this whole file is actually generated by the master/scripts/generators/asciidoc_fields.py file. The templates are below line 142. However the logic to decide when to show this will have to be in the code above that. Do you want to try your hand at playing in the Python code? If that's out of the cards I can take it from here. If you'd like to try this, we can have a quick discussion and I show you around the code. WDYT? |
As discussed yesterday, here's the code part that you'd need to integrate in the script file to only get the note to appear when there's field reuse going on. Add this function above the definition of function def render_fieldset_reuse_link(fieldset):
'''Render a link to field reuse section, only when appropriate'''
if ('nestings' in fieldset or 'reusable' in fieldset):
return 'NOTE: ...'
else:
return '' You'll add the result of calling this function in the def render_fieldset(fieldset, ecs_nested):
text = field_details_table_header().format(
fieldset_title=fieldset['title'],
fieldset_name=fieldset['name'],
fieldset_description=render_asciidoc_paragraphs(fieldset['description']),
fieldset_reuse_links=render_fieldset_reuse_link(fieldset)
) And finally, in the template function {fieldset_description}
{fieldset_reuse_links} With this in place, we can now iterate on how this looks by tweaking what's in In the future we can collaborate more smoothly on this if you give me push access to your fork. This way I could directly push some code changes to your PRs, and you could focus on shaping the text & so on. We can discuss this when you're back. |
So previous comment was about the code needed for this. Now let's also have some actual documentation discussion :-) Only feedback there I think would be to turn this into more of a full sentence, with only part of it being the actual link. With this in mind, there's a few situations to consider:
Perhaps we can come up with a generic sentence for situations 1, 2 and 3: Or perhaps we can adjust the wording depending on whether it's 1, 2 or 3? What do you think? Also, I think it's fine to have nothing when there's no reuse, but this is open for discussion as well. Once we're done with the NOTE at the top, we could work on adjusting the "field reuse" section itself, as well. But I'd keep that for a subsequent PR :-) |
@webmat @benskelker Since ECS is a specification, explicit documentation saying something like "This field set is not nested." will be very helpful for readers who might not read all field set definitions to even realize that nesting is a thing. Can we add such a sentence for the fourth case? |
@MikePaquette Yes, this makes sense. Let's err on the side of being more explicit 👍 |
@webmat @MikePaquette h1: Fieldset name h3: Field Reuse
h2: Field Details This way, all the information required for a fieldset is presented before describing each field. |
@benskelker Could you mock it up and show the result? Just a screenshot of the rendered asciidoc is enough. In other words, no need to update the code for this; just tweak one page manually, render with asciidoctor & paste here as a comment. |
The doc builds fine locally on my machine |
@benskelker Sorry I meant a screenshot of the page in the browser :-) |
Wait, I think I misunderstood what you were asking for in this comment #569 (comment). I thought you were suggesting we try your initial idea of adding all of the field reuse section at the top. But what you're showing me in these code screenshots is the approach I'm ok with. If that's what you want to go for, I'm all for this, no need to show me a POC :-) |
@webmat @benskelker I too thought you were proposing putting all the re-use info up top after the fieldset definition. I am OK with either approach. |
We're good to go - just updated the text a bit in the last commit. |
The build is failing because of the Python code formatter. Running |
I'm not sure about the big interstitial saying "Not reused" being there 100% of the time, though. Especially not for the "Base" fields. I feel like people will tune it out, if there's always this note that 75% of the time says "not reused". I think having the "Field Set" section always present at the end, stating that these fields are not reused elsewhere nor anything nested under here would make more sense. Although I would tackle that in a separate PR. I'd rather keep this PR only focused on the notice at the top of the page. I also think the wording when there is reuse could be simplified. Out of context, I'm not sure how the current sentence speaks to people. "field set" is a bit inside baseball, even if we use it in the docs. Although when trying to simplify it and use more common words, I quickly get to a point of needing distinct sentences for each situation.
|
If you agree with this kind of approach (and we can improve the wording together), I will adjust the code. Let me know :-) |
This commit goes much further than the original PR. I had a stab at updating the code - hope that's ok. I ran |
3e76626
to
754a903
Compare
You can preview the changes. |
This is looking good @benskelker. I still think we need some mention that a fieldset is not expected to be nested under other field sets. For example based on this latest commit, if I look at the host fieldset definition here, it is silent about whether It would be clearer and helpful if the note at the the bottom was enhanced to say: |
@MikePaquette |
Yeah I like the short note at the top, as you propose it. I think the wording works well no matter what kind of nesting is happening. I also like that you adjust the wording "can" & "must", depending on whether the field set is expected at the root or nested, or exclusively nested. Good stuff 👍 Let's continue working on the wording a bit, however. I think "These fields are not reused." is too short, and doesn't give enough context, for someone who isn't sure what this "field reuse" stuff is about. I think we need to flesh it out more, maybe something like "The {field set} fields are not expected to be nested under another field set." I like that you're strengthening the top_level note by using "should not" instead of "are not expected". However I would not use the expression "root fields", I think it's going to be confusing: it almost sounds like The build is failing because the generated asciidoc files are not fully committed. If you run |
Also what @MikePaquette points out is true. There happens to be fields nested under host, so we have a section talking about nested fields under host. But the page should still say that "host" should not be nested elsewhere. In other words, the "Field Reuse" section should always be present (already done). But perhaps it should always have two subsections as well: Field reuseFields Nested in {fieldset_name}Nesting {fieldset_name} Fields in Other FieldsAgent example (no nesting) Field reuseFields Nested in AgentNo other ECS fields are nested in Nesting Agent Fields in Other FieldsThe Host example (nest here only) Field reuseFields Nested in HostThe
Nesting Host Fields in Other FieldsThe Group example (nest elsewhere only, top level or nested) Field reuseFields Nested in GroupNo other ECS fields are nested in Nesting Group Fields in Other FieldsThe
NOTE: The "Autonomous System" example (nest elsewhere only, not at top level) Field reuseFields Nested in GroupNo other ECS fields are nested in Nesting Autonomous System Fields in Other FieldsThe
NOTE: The "User" example (nesting here, and nesting elsewhere) Field reuseFields Nested in UserThe
Nesting User Fields in Other FieldsThe
NOTE: The |
I used notes for clarifying when field sets cannot be nested and updated the text when there is no nesting at all. |
6c96175
to
3fed4fc
Compare
@webmat @MikePaquette |
@benskelker I really like this approach. It provides the valuable nesting information for every field set, and is a great improvement in our docs. I am fine to go ahead with this as it is. If we want to spend further time polishing this, I have three observations:
Suggestions:
Example (italics indicate suggested changes): Other field sets may be nested under the host field as follows:
The host fields must not be nested under other field sets. |
Nesting does not explicitly describe the hierarchy, which leads to awkward sentences. Are you ok with 'fields can be children of'?
Yea, I thought about that but I think it's pretty self-explanatory (might be wrong).
If we must, I I'd love to customise the desciptions depending on the context, but let's leave that for a different PR. @webmat any thoughts? |
100% agree that contextual descriptions would be great and I also agree with keeping that for later :-) In order to do this, we'll need to 1) modify the YAML structure to allow for these additional contextual descriptions 2) fill it in for all field reuses where it's needed 3) adjust the scripts to leverage them. Actually I like the idea so much that it's one of the first ideas I added to the ECS docs brainstorm document, a few weeks ago (it's #4). On wording:
Field sets that have nothing nested inside them but are nested elsewhere (e.g. group) are not stating that "this field set has no other fields nested inside it". Note that this is only true for fields that are nested elsewhere. Fields that have zero nesting whatsoever are fine on this front, because they have the catch-all sentence. Fields that have zero nesting (e.g. network) could still use a small wording adjustment, because the sentence mixes both metaphors:
I wonder if we couldn't instead use only one metaphor for this sentence. That, or consider again my initial proposal to always have two sub-sections inside "Field Reuse". In this case, both sections contain a simple sentence: Fields Nested in AgentNo other ECS fields are nested in Nesting Agent Fields in Other FieldsThe |
@MikePaquette @webmat I'll make one last plea not to use RFC wording :). May has connotations of chance and choice, whereas 'can' implies capability and purpose. I also think the recommendation is not relevant for our use case. We are not providing optionality, we are providing reusability. If users have to map geo fields, they must be nested. From the memo:
Let me know. Let's try and agree on the nesting vs child/parent terminology. From your comments, I suggest we only use child/parent and never nested/nesting. Please let me know if you're okay with this.
Yes: Lastly:
In these cases, let's add a bullet to the note: NOTE: The
|
@benskelker Thanks!
|
@benskelker Ah sorry I hadn't picked up on the fact that you were asking not to use the RFC wording. From Mike's comment above, seems like he hasn't either 😂 I get what you're saying about "may", however I think "must not" is much clearer than "shoudn't". I don't think we need to make this a big issue. On nesting vs parent/child: I'm wondering if we should table this discussion and move it to the brainstorming doc for now. The reasoning is that now we're looking at one small section of the docs; but if we want to eliminate one of the two metaphors, we should look around the docs and think about this holistically. I don't want to block this PR -- meant to point people to the "reuse" section -- with this holistic review. And I wouldn't want to eliminate one metaphor now by focusing only on this section, and upon further review decide to bring it back because it made more sense elsewhere. The secondary reason is that I don't think solely using "parent/child" is clear enough. I think introduced the Parent/Child metaphor for the column names, where this made sense to describe the position of the fields, in the context of talking about nesting. But seeing a sentence stating "this field set is not a parent of other field sets" doesn't speak to me as much. If someone new to ECS asked me "what does it mean to say a field set is a parent of another?" I'm not sure I could explain it without talking about nesting. It seems to me like nesting is the fundamental concept here. Are we ok to table the nesting vs parent/child discussion? If so, I'll add this to the brainstorming doc. We should still adjust the sentence that uses two metaphors, however. Options:
My favorite one is the last, as it's most in line with the wording in the ECS documentation right now. I'm ok with not having subsections, though. |
@webmat and @MikePaquette |
dac8234
to
973ae30
Compare
This PR has been stale for a while, and I believe a lot of the discussed functionality has since been introduced through other contributions. |
Just added the field reuse link to
as
fields. If we're good to go, I'll add it to all relevant field sets in this PR.