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

Allow plain strings in defines #27

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LecrisUT
Copy link

@LecrisUT LecrisUT commented Jul 5, 2023

This allows to pass -DFOO=some_value

@LecrisUT LecrisUT mentioned this pull request Jul 5, 2023
@aradi
Copy link
Owner

aradi commented Aug 8, 2023

Thanks for the PR! I am somewhat unsure about this change, as it would change the user experience on typos. -DLOGICAL=false causes currently an error, while with you change, it would not generate an error, but instead create a variable LOGICAL which evaluates to True!

Is there any use case, where the additional quoting causes significant problems (apart of convenience)?

@LecrisUT
Copy link
Author

LecrisUT commented Aug 8, 2023

I think the issue is that it counter to how the compilers are using define. There it is understood that -DSomething will create a definition of Something (as string), and not that it will evaluate a pythonic syntax. I believe for that case it is better to use a different flag like -P for parse.

As for the use-case, this will allow to transparently pass the pre-compile flags from one target to the other, e.g. in the cmake:
https://github.com/aradi/fypp/pull/28/files#diff-596e5008f61a102e76c56394cda92747f836cce43620836bf97838608c482e24R272-R273
(i.e., these flags can be added/fixed after the definition of the target, which is common for nested cmake projects)

@aradi
Copy link
Owner

aradi commented Aug 8, 2023

I agree, that it was probably counterintuitive to abuse -D to evaluate Python syntax and something like -P would have been a better choice (sorry!). However, changing it now would brake a lot of existing code, which I'd like to avoid.

What about an extra option to settle this? Something like -d or --define-value-type which controls how define values are interpreted, e.g. --define-value-type=expr (or -d expr) would evaluate it as Python expression, while --define-value-type=str (or -d str) as string literal. By keeping former as default, we would remain backwards compatible, while latter could be specified if no Python evaluation is required. (Actually, it maybe even enough just to use a single switch, like -s, which would turn the string interpretation on, when specified, as I don't think we will have other define value types as the current and the string one...)

@LecrisUT
Copy link
Author

LecrisUT commented Aug 8, 2023

Indeed that would work. I would add though that there should be a deprecation message and transition for 3.3 or later where the default will change to the more natural syntax. Probably the documentation page should be improved and split for some time before that though so that it can be more visible there.

@aradi
Copy link
Owner

aradi commented Aug 9, 2023

OK, let's do the following:

  • Introducing -d (--define-value-type), which can be set to str or python, with latter being default. If former is set, the values in the defines are interpreted as string literals. This makes the interpretation of the -D options depend on an other option, but I think, we can live with that.
  • Introducing the -P (--python-define) option, which is equivalent to the current -D (defines with Python expression as value). The behavior of -P is independent of the value set with -d.
  • As a preparation for a change, we could at some point start to emit deprecation messages, if the -d option is not set. This way, hopefully everyone sets the -d option explicitly, so if its default value ever changes, most projects should not need any changes.
  • Changing the default of -d to str (release 4.x, as we would brake backwards compatibility).

I'd need some time, though, to implement it.

- For backwards compatibility, -D will continue to be interpreted as Python expression until 4.0
- Introduced a temporary variable -d/--define-value-type to switch to the proper interpretation when set to `str`

Signed-off-by: Cristian Le <[email protected]>
@LecrisUT
Copy link
Author

LecrisUT commented Sep 8, 2023

I've come back to add the discussed implementation. @aradi I find the python tests quite hard to follow, could you add the necessary tests? I'll try to clean them up later in #29 (or rather in a subsequent PR, so that one can get merged faster)

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.

2 participants