Skip to content

Pure static classes to namespaces#17812

Merged
jmarantz merged 2 commits intoenvoyproxy:mainfrom
capoferro:pure_static_classes_to_namespaces
Aug 31, 2021
Merged

Pure static classes to namespaces#17812
jmarantz merged 2 commits intoenvoyproxy:mainfrom
capoferro:pure_static_classes_to_namespaces

Conversation

@capoferro
Copy link
Contributor

@capoferro capoferro commented Aug 23, 2021

Commit Message: Reviewer prefers namespaces to static classes (Envoy style guide doesn't mention this).
Additional Description: This is a followup from comments in #17728
Risk Level: Very Low
Testing: Existing

Signed-off-by: Josiah Kiehl <josiah@capoferro.net>
Signed-off-by: Josiah Kiehl <josiah@capoferro.net>
@capoferro
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17812 (comment) was created by @capoferro.

see: more, trace.

@jmarantz
Copy link
Contributor

"Envoy prefers namespaces to static classes." --> citation needed :)

I can't find any reference to that in https://github.com/envoyproxy/envoy/blob/main/STYLE.md -- and I thought it was plenty common to use statics in a class in Envoy.

@capoferro
Copy link
Contributor Author

capoferro commented Aug 24, 2021

"Envoy prefers namespaces to static classes." --> citation needed :)

Oh... I got a comment on the last PR that I should use a namespace instead. I don't personally have a preference.

#17728 (comment)

@jmarantz
Copy link
Contributor

Heh -- I personally prefer static class methods for various reasons, but don't feel that strongly. I'll defer to @dmitri-d :)

@jmarantz jmarantz assigned dmitri-d and unassigned jmarantz Aug 24, 2021
@dmitri-d
Copy link
Contributor

Haha, I don't actually feel strongly about this either, I thought that was the preferred way for grouping of static functions in Envoy. Lgtm.

@capoferro
Copy link
Contributor Author

@jmarantz Are you the one to hit the merge button?

@jmarantz
Copy link
Contributor

I'm OK merging this but IMO this is not at all preferred by Envoy style, for the record. I think Envoy is OK with either one and there are a ton of existing uses of classes with static members, and there are some benefits to that approach.

@jmarantz jmarantz merged commit b45e5f3 into envoyproxy:main Aug 31, 2021
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.

3 participants