You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2015/06/09 20:19:27 UTC
Re: Review Request 34361: converted hard-coded strings to consts
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34361/#review87248
-----------------------------------------------------------
Hey Colin!
Thanks for contributing this and I am sorry about the tardy turn-around time.
The crux of the ticket was to consolidate the strings, so we only have to maintain them one place.
With that in mind; instead of inlining the string constants, can we add them to an appropiate header as 'const char[]'?
@Vinod: Would it make sense to add this to src/tests/mesos.hpp and src/tests/mesos.cpp, as the label tests are in module, master and slave tests?
- Niklas Nielsen
On June 8, 2015, 12:05 p.m., Colin Williams wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34361/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 12:05 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2637
> https://issues.apache.org/jira/browse/MESOS-2637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> converted hard-coded strings to consts
>
>
> Diffs
> -----
>
> src/tests/hook_tests.cpp 3ffde6d
> src/tests/master_tests.cpp ba3858f
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/34361/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Williams
>
>
Re: Review Request 34361: converted hard-coded strings to consts
Posted by Colin Williams <la...@gmail.com>.
> On June 9, 2015, 6:19 p.m., Niklas Nielsen wrote:
> > Hey Colin!
> >
> > Thanks for contributing this and I am sorry about the tardy turn-around time.
> >
> > The crux of the ticket was to consolidate the strings, so we only have to maintain them one place.
> > With that in mind; instead of inlining the string constants, can we add them to an appropiate header as 'const char[]'?
> >
> > @Vinod: Would it make sense to add this to src/tests/mesos.hpp and src/tests/mesos.cpp, as the label tests are in module, master and slave tests?
>
> Colin Williams wrote:
> It made sense to me to connect the values that were actually related, but it seemed like many of these strings shared a value only because "foo" and "bar" are really common test values. I thought it would be misleading to consolidate the strings for which it didn't matter if they matched, am I missing a connection somewhere?
Actually, there seemed to be a connection between src/tests/hook_tests.cpp and src/examples/test_hook_modules.cpp, but I couldn't find it in the code. It also looked like the convention had already been established between those files of keeping them in sync with a series of variable declarations at the top of each file. There was some negative feedback with those changes, though, so I reverted them.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34361/#review87248
-----------------------------------------------------------
On June 8, 2015, 7:05 p.m., Colin Williams wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34361/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 7:05 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2637
> https://issues.apache.org/jira/browse/MESOS-2637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> converted hard-coded strings to consts
>
>
> Diffs
> -----
>
> src/tests/hook_tests.cpp 3ffde6d
> src/tests/master_tests.cpp ba3858f
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/34361/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Williams
>
>
Re: Review Request 34361: converted hard-coded strings to consts
Posted by Colin Williams <la...@gmail.com>.
> On June 9, 2015, 6:19 p.m., Niklas Nielsen wrote:
> > Hey Colin!
> >
> > Thanks for contributing this and I am sorry about the tardy turn-around time.
> >
> > The crux of the ticket was to consolidate the strings, so we only have to maintain them one place.
> > With that in mind; instead of inlining the string constants, can we add them to an appropiate header as 'const char[]'?
> >
> > @Vinod: Would it make sense to add this to src/tests/mesos.hpp and src/tests/mesos.cpp, as the label tests are in module, master and slave tests?
It made sense to me to connect the values that were actually related, but it seemed like many of these strings shared a value only because "foo" and "bar" are really common test values. I thought it would be misleading to consolidate the strings for which it didn't matter if they matched, am I missing a connection somewhere?
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34361/#review87248
-----------------------------------------------------------
On June 8, 2015, 7:05 p.m., Colin Williams wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34361/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 7:05 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2637
> https://issues.apache.org/jira/browse/MESOS-2637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> converted hard-coded strings to consts
>
>
> Diffs
> -----
>
> src/tests/hook_tests.cpp 3ffde6d
> src/tests/master_tests.cpp ba3858f
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/34361/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Williams
>
>