mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 34375: Removed use of namespace aliases.
Date Mon, 15 Jun 2015 18:24:01 GMT


> On May 19, 2015, 2:02 a.m., Ben Mahler wrote:
> > This might be a valid namespace alias use case that we hadn't considered, because
there is no way to be able to write just `http::Response` otherwise, is there?
> > 
> > Seems quite verbose to write process::http everywhere, and on the otherhand just
having `Request` or `Response` seems to miss the context of it being http, thoughts?
> 
> Michael Park wrote:
>     > This might be a valid namespace alias use case that we hadn't considered
>     
>     I actually think we should reconsider the overall approach of disallowing aliases
(e.g. type, namespace).
>     > because there is no way to be able to write just http::Response otherwise, is
there?
>     
>     We could do so by declaring `using namespace process;` if we wanted to.
>     > Seems quite verbose to write process::http everywhere and on the otherhand just
having Request or Response seems to miss the context of it being http
>     
>     This is the entire point of aliases in general, which is why I think we should reconsider
the decision to disallow them.
>     > thoughts?
>     
>     From a offline discussion with BenH, it seems like the decision to disallow type
and namespace aliases to avoid inconsistencies in different files. For example, one person
could write `namespace http = process::http;` and another person could write `namespace phttp
= process::http;`.
>     
>     I think the correct solution is to come up with naming guidelines to give them good
and consistent names, rather than disallowing the feature. For example, we don't disallow
functions just because "someone could possibly give it a horrible name". We instead try to
influence people to give their functions meaningful names in a consistent manner. An example
of such rule is that we write `member()` rather than `getMember()` for our getter functions.
We don't simply disallow getter functions just because people could say them inconsistently.
>     
>     Anyway, given the current rules, I would push for us to be consistent with no special
cases. Especially since we have much longer names all over in the codebase that we cope with.
(e.g. `mesos::master::allocator::Allocator`, `&master::allocator::MesosAllocatorProcess::deactivateFramework`,
`master::allocator::HierarchicalDRFAllocator`)
>     
>     I'm definitely interested in starting the discussion around reconsidering the rules
and perhaps formalizing some of the naming rules around aliases, but I also think that that
should happen outside of this review, probably in a JIRA ticket.

Sounds great Michael! Happy to help you get these aliases added, at least restricted to this
use case initially. :)


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34375/#review84251
-----------------------------------------------------------


On June 13, 2015, 9:51 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34375/
> -----------------------------------------------------------
> 
> (Updated June 13, 2015, 9:51 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 3049e057c87571f5db73ee0b14db8b47132b2dff 
>   src/slave/slave.cpp 9af69d8f0b28c9441c684886c52320378f9b2869 
>   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
>   src/tests/slave_tests.cpp 8dae6e0842c2bedddc13d1c036390e85c7a96df7 
> 
> Diff: https://reviews.apache.org/r/34375/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message