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

Add column 'level' to display which field is core/extended #149

Merged
merged 7 commits into from
Oct 29, 2018

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Oct 25, 2018

Also in this PR

  • Get rid of the concept of phase, which was no longer used
  • Some variable renaming in use-cases.py write_stdout(), to improve readability
  • Fix a few use case fields that should now be keyword
  • Fix a few use cases for host.hostname rename

Caveat:

  • As of #9ee4ceb, the code to generate use cases completely breaks
    if there's a multi_fields anywhere. This is true whether the mf is only
    in the use case (e.g. a customization) or in cases where the field is both in
    ECS and in the use case.

Partially addresses #93

@webmat webmat requested a review from ruflin October 25, 2018 02:29
@webmat webmat self-assigned this Oct 25, 2018
@webmat
Copy link
Contributor Author

webmat commented Oct 25, 2018

Folks, here's the URL to browse this PR as if it was pushed to master, to look at the readme and use cases: https://github.com/webmat/ecs/blob/level-scripts

@webmat
Copy link
Contributor Author

webmat commented Oct 25, 2018

About the caveat, I'd be inclined to open a GH issue and get this resolved when we get back to having a use case or an ECS field that's multi_fields. WDYT?

@webmat webmat mentioned this pull request Oct 25, 2018
26 tasks
@webmat
Copy link
Contributor Author

webmat commented Oct 25, 2018

Link to browse above is incorrect. Use this one to view the rendered readme: https://github.com/webmat/ecs/tree/level-scripts

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Seems like Examples are not shown anymore with this change?

@@ -137,7 +134,7 @@ def get_markdown_table(namespace, title_prefix="##", link=False):
# Replaces one newlines with two as otherwise double newlines do not show up in markdown
output += namespace["description"].replace("\n", "\n\n") + "\n"

titles = ["Field", "Description", "Type", "Multi Field", "Example"]
titles = ["Field", "Level", "Description", "Type", "Multi Field", "Example"]
Copy link
Member

Choose a reason for hiding this comment

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

I would put the level after the Type.

As we don't have multi fields anymore, we can remove the multi field part? Or should we keep it just in case. Not related to this PR but stumbled over it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree level at column 2 is not ideal.

However putting level after type puts it between type and multi-field, which are closely related. I'll try with level just before type instead, and let's see how this looks.

@@ -95,12 +95,14 @@ def check_fields(fields):

check_fields(sortedNamespaces)

# Generates html for README
Copy link
Member

Choose a reason for hiding this comment

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

It creates Markdown not HTML :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha true :-)

@webmat
Copy link
Contributor Author

webmat commented Oct 26, 2018

@ruflin This is ready for a review. And I'd like your opinion on the caveat for the PR as it stands (see PR description)

@webmat
Copy link
Contributor Author

webmat commented Oct 26, 2018

You're right about the examples, damn! Not sure how I missed that :-)

  • Actually display example column content in readme, again
  • Move "level" column between description and type

@webmat
Copy link
Contributor Author

webmat commented Oct 26, 2018

@ruflin Ready for another review. The two issues (and incorrect comment) are fixed:

https://github.com/webmat/ecs/blob/level-scripts/README.md#-base-fields
https://github.com/webmat/ecs/tree/level-scripts/use-cases

However here's an experiment I made at the same time:

  • Removed the "multi field" column (right now there's no MF in ECS, and when there is, they are displayed on their own line anyway)
  • Move columns of predictable width before description (level and type)

I find this to be much more readable, check it out here:

https://github.com/webmat/ecs/blob/level-tbl-display-experiment/README.md#-base-fields

Unless you have reservations about this, I would really like to move to this representation.

@ruflin
Copy link
Member

ruflin commented Oct 29, 2018

I agree variable column width is not optimal but as already the first column is variable I would rather focus on the importance of the information in ordering it. For me priority is:

  • field
  • description
  • type

Then somewhere much later comes

  • level
  • example

So I would definitively keep the order of the first 3.

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I'm good with the change as is to get it in. We can still rearrange columns in a follow up PR.

@webmat webmat merged commit b1f31eb into elastic:master Oct 29, 2018
@webmat webmat deleted the level-scripts branch October 29, 2018 13:44
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.

2 participants