The starting code for Last Will on the C++ track is very confusing because the namespaces are not indented and they are nested. I’m not sure if this is intentional or not, but it made it so I couldn’t understand the structure at all until I indented it myself (the comments like // namespace red did not help much).
I think it would be a good idea to indent it properly, especially because it is a concept exercise that beginners could encounter early on (and the point of the exercise is to teach namespaces, not indentation).
The lack of indentation also bothered me in the “Hello World” exercise, but that one was still comprehensible, unlike Last Will.
I’m fairly certain that is not intentional. The indentation somehow got dropped in this jumbo PR by @ahans. The stub should definitely have proper indentation added back!
Looking through the diff, I see that the exemplar solution got messed up too… seems like it was definitely an accident.
Should I wait for three maintainers to consent, or should I start drafting a PR now because it’s a small change? I’m still uncertain of how exactly the system works.
Practice exercises are synced from the problem specs to specific tracks. This includes the markdown text and the test data. Changes to the problem specs requires sign of from three maintainers.
Concept exercises and the actual code files in specific track/language repos are managed by the track maintainers. Different maintainers have different preferences around PRs. @ahans is one of the C++ maintainer and can speak authoritatively about what should happen here.
For quite a while tooling around automatically applying clang-format to all code files was broken and it did not format all files. The mentioned jumbo PR fixed this, but as it turns out also introduced some inconsistencies, which we obviously have to fix. The question is how.
The clang-format configuration we use makes clang-format not indent namespace bodies. This is why the indentation was removed from those concept exercise files. In general, I think this makes sense and should be kept that way. Indenting each namspace body would help readability regarding which namespace code belongs to, but in most cases it would add unnecessary whitespace and actually hurt readability. Cases like the one from the exercise, where you have code on different namespace levels, are actually quite rare. What you typically have is something like this:
namespace a {
namespace b {
namespace c {
int my_func();
}
}
}
Indenting here would move the actual declarations far to the right without adding much benefit.
I believe cases like this triggered the introduction of nested namespace in C++17, which allows expressing the above more concisely:
namespace a::b::c {
int my_func();
}
This is not mentioned in the concept and concept exercise at all it seems.
At the very least, we should make the styles consistent by removing the indentation from the code samples in the .md files. I think we should also extend the concept to also mention C++17 nested namespaces. We could switch the exercise to using nested namespaces as above, that would probably reduce confusion. To teach students that there are both ways, we could keep a part of the exercise using the old style. Not sure if we would want to put indentation back. If we did, we’d have to put some // clang-format off comments around those parts.
I will put together a PR with fixes. But before that, happy to hear the thoughts of others!
Using the nested namespace syntax sounds like a great idea!
If the exercise keeps some of the previous style to show the two different ways, I like the idea of indenting it and using // clang-format off there, as the different levels of nesting is really confusing without indentation.
Also, I have a suggestion for consistency: In Log Levels, there is no empty line between the namespace and function (while in many other places there is):
Agree with the OP, after indenting the namespaces myself it was easier to read although I’ve never seen embedded namespaces before, so that was enlightening
@ahans, any updates on this? It seems like the styles are still inconsistent, with the template being unindented and the code sample in introduction.md being indented.
