GobySoft / dccl

Dynamic Compact Control Language
Other
17 stars 13 forks source link

4.0 - Query: full_width in dccl/src/codec.cpp #83

Closed psmskelton closed 2 years ago

psmskelton commented 2 years ago

Questions:

  1. Does full_width have any purpose other than console output formatting? I can't find any other purpose than programmatic generation of guard, header_guard, body_guard, and codec_guard.
  2. Why is full_width outside of the dccl namespace?
  3. Why is full_width set to 60 characters? We have fully qualified field names that are legal, yet with the inclusion of {dccl.default.id} on the console output they exceed 60 characters, so the guarded outputs are shorter than the field outputs, which detracts from readability.

Proposal:

  1. Make full_width a 'private' member of dccl::Codec.
  2. Add default assignment in dccl::Codec constructor for fullwidth(60) so that no unexpected changes occur to behaviour for users.
  3. Provide 'public' function dccl::Codec::set_full_width(unsigned) for user control of output formatting widths.

We have a project where, rightly or wrongly, our message definitions are deep enough in nested packages that we are running into problems with this, and I don't want to have to resort to arbitrary acronyms/initialisms/abbreviations which will greatly reduce readability and comprehension.

@tsaubergine - I'm happy to create the patch and pull request, but wanted to see if you had any problems before doing so.

tsaubergine commented 2 years ago

Hi -

Questions:

  1. No it's simply for formatting the console output.
  2. Sloppiness and the fact that it exists only in codec.cpp (so its scope is somewhat limited).
  3. 60 chars is readable on most consoles, but it's a bit arbitrary, I'll admit.

Proposal:

  1. Sure, let's rename it to console_width or something similar which will be less vague.
  2. Sounds good
  3. Also sounds good, though maybe set_console_width or similar (based on your choice in 1.).

Definitely happy to review a PR for this, thanks!

psmskelton commented 2 years ago

@tsaubergine - Any problems with #84?

tsaubergine commented 2 years ago

@tsaubergine - Any problems with #84?

Sorry, I had thought I had merged that.