Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a CLI flag to disable the snake_case conversion of message and signal names #255

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tjhowse
Copy link
Collaborator

@tjhowse tjhowse commented Jan 18, 2021

We have a use case in which we wish to preserve message and signal names in the original form in the generated C source code. This PR adds a CLI flag that disables the default behaviour of converting all message and signal names to snake_case.

It ads a guard around each use of camel_to_snake_case(). If disable_snake_case_conversion is true then we only perform the _canonical() sanitation. It also changes self.snake_name to self.exported_name for messages and signals.

A cleaner solution would be to create a generator class which stores the settings for the generation, and make all generator functions methods of the new class, rather than passing the new argument into every function in the call tree that includes camel_to_snake_case, but that would be a bigger architectural change.

If this is a change that other people are likely to find useful, let me know and I'll finish off this PR. As it stands we'll probably continue to use my fork internally.

  • WIP
  • Add tests
  • Update readme

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.02%) to 93.168% when pulling 02acb49 on tjhowse:Optional_snake_case_signal_names into cd791f4 on eerimoq:master.

Copy link
Member

@andlaus andlaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would probably be better move the stuff which concerns itself with C code generation out of the database part of the codebase into the relevant subparser? (side note: the class names here are quite confusing, IMO.)

besides this, the patch seems to be fine...

@@ -536,9 +536,13 @@

class Signal(object):

def __init__(self, signal):
def __init__(self, signal, disable_snake_case_conversion):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is probably slightly better to use a "positive" name for the parameter, e.g. use_raw_names?

@simoneruffini
Copy link

What's the status of this PR? I'm interested in it being merged

@andlaus
Copy link
Member

andlaus commented Jul 13, 2023

What's the status of this PR? I'm interested in it being merged

it currently has some merge conflicts with master...

Co-authored-by: Andreas Lauser <github@poware.org>
@andlaus
Copy link
Member

andlaus commented Jul 13, 2023

... and it would be nice if the functionality was featured by a unit test. Besides this, I don't see an reason not to include this functionality. (It looks like this PR was left unmerged because the discussion does not seem to be finished, then it started to suffer from bitrot after a while...)

@simoneruffini
Copy link

So we are missing unit test and it can be merged?

@andlaus
Copy link
Member

andlaus commented Jul 13, 2023

So we are missing unit test and it can be merged?

plus a rebase on top of the latest master...

@simoneruffini
Copy link

If the original author of the PR doesn't rebase, can someone else do it?

@andlaus
Copy link
Member

andlaus commented Jul 16, 2023

If the original author of the PR doesn't rebase, can someone else do it?

yes. just make a new PR and mention that it is based on this one...

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.

4 participants