Skip to content

Improve and simplify generation of fields#10168

Merged
ruflin merged 8 commits intoelastic:masterfrom
ruflin:asset-name
Jan 24, 2019
Merged

Improve and simplify generation of fields#10168
ruflin merged 8 commits intoelastic:masterfrom
ruflin:asset-name

Conversation

@ruflin
Copy link
Copy Markdown
Contributor

@ruflin ruflin commented Jan 18, 2019

Introduce method that all Method names start same way. Unify asset generation to remove duplicated code.

@ruflin ruflin added module review Team:Integrations Label for the Integrations team labels Jan 18, 2019
@ruflin ruflin self-assigned this Jan 18, 2019
@ruflin ruflin requested review from a team as code owners January 18, 2019 14:10
@ruflin ruflin force-pushed the asset-name branch 2 times, most recently from 1e96b99 to 9a9d5c0 Compare January 21, 2019 15:44
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why using camel cased names here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is to better match with the naming scheme of Go functions, like FieldsYml as an example.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ruflin This might not be necessary in this PR because it's applied to a different value than before. IIRC previously this was being used to camel case the generated Go function name like AssetFileIntegrity() rather than AssetFile_integrity(). And now it appears to be mutating the source file name that's referenced in the generated code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@andrewkroh It will be the case in a follow up PR. I just copied it over from there and is the reason it does not look optimal here. The more important point of this PR is bringing it into one place so will make it look good for this PR and can then still adjust it in the follow up PR.

@ruflin
Copy link
Copy Markdown
Contributor Author

ruflin commented Jan 23, 2019

@jsoriano @andrewkroh I updated this PR again to also include the support for multiple assets per package even though it's not needed yet and only in the follow up PR. I did it here to have fewer changes in the follow up PR. The paths and names in some of the files are not optimal but work. I suggest to leave it like this for now as all these will change again with #9507

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ugh, that is ugly. need to fix this.

@ruflin ruflin merged commit 04b97e8 into elastic:master Jan 24, 2019
@ruflin ruflin deleted the asset-name branch January 24, 2019 08:23
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
Introduce method that all Method names start same way. Unify asset generation to remove duplicated code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module review Team:Integrations Label for the Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants