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

Feature: make ecs more flexible #2019

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

maihde
Copy link
Contributor

@maihde maihde commented Jul 29, 2022

When using the ECS tooling for custom projects, there are a few minor changes that make would make the tooling more flexible, especially in cases where the ECS schema itself might be minimally used. I appreciate that the primary use case for ECS tooling is the ECS schema, but given that changes are minor and their is value of increased utility for the tooling I hope they could be considered for inclusion in the baseline.

I welcome discussion on the proposed changes.

@maihde maihde requested a review from a team as a code owner July 29, 2022 18:51
@maihde maihde force-pushed the feat-make-ecs-more-flexible branch from 77dfe53 to 46968b0 Compare August 7, 2022 21:46
@github-actions
Copy link

This PR is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale Stale issues and pull requests label Oct 10, 2022
Copy link

@IanLee1521 IanLee1521 left a comment

Choose a reason for hiding this comment

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

LGTM, though I'm not a user of this tool, so I'm possibly missing something.

Comment on lines 38 to 45
def generate(
ecs_nested: Dict[str, FieldNestedEntry],
ecs_version: str,
ecs_component_name_prefix: str,
out_dir: str,
mapping_settings_file: str,
template_settings_file: str
) -> None:

Choose a reason for hiding this comment

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

Not really having context for this tool, adding a new argument like this in the middle seems like it might not be the most compatible way of doing this.. Usually I'd expect new arguments to show up in the kwargs section of the list and then have a reasonable default, which in this case looks like it should be ecs:

Suggested change
def generate(
ecs_nested: Dict[str, FieldNestedEntry],
ecs_version: str,
ecs_component_name_prefix: str,
out_dir: str,
mapping_settings_file: str,
template_settings_file: str
) -> None:
def generate(
ecs_nested: Dict[str, FieldNestedEntry],
ecs_version: str,
out_dir: str,
mapping_settings_file: str,
template_settings_file: str,
ecs_component_name_prefix: str = "ecs",
) -> None:

Choose a reason for hiding this comment

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

Moved to kwarg

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ralphm ralphm left a comment

Choose a reason for hiding this comment

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

It would be useful to detail what improvements you made from a user's perspective, as this is not detailed on the PR description. The individual commit messages do give a short indication, but also per the contribution guide, could you please add to CHANGELOG.next.md?

@desean1625
Copy link

This pull request allows users to

  1. Define their own template documentations for ASCII DOCS
  2. Run ECS from a different directory. This allows ECS to be included in a project as a submodule
  3. Set a component name that isn't ecs_

This allows ECS tooling to be utilized by more organizations for generating their component templates, and documentation.

@github-actions github-actions bot removed the stale Stale issues and pull requests label Feb 29, 2024
Copy link

This PR is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale Stale issues and pull requests label Apr 29, 2024
@github-actions github-actions bot removed the stale Stale issues and pull requests label Aug 22, 2024
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.

5 participants