[riot-notifications] [RIOT-OS/RIOT] make: add LOG_LEVEL to overridable variables (#11592)

Martine Lenders notifications at github.com
Tue May 28 11:42:09 CEST 2019


miri64 requested changes on this pull request.



> @@ -95,3 +95,5 @@ export UNZIP_HERE            # Use `cd $(SOME_FOLDER) && $(UNZIP_HERE) $(SOME_FI
 
 export LAZYSPONGE            # Command saving stdin to a file only on content update.
 export LAZYSPONGE_FLAGS      # Parameters supplied to LAZYSPONGE.
+
+# LOG_LEVEL                  # Logging level (NONE: 0, ERROR: 1, WARNING: 2, INFO: 3, DEBUG: 4, default: 3)

Not sure this is maybe a little bit ambiguous. This could be read, that log levels are to be given as words. Maybe write
```suggestion
# LOG_LEVEL                  # Logging level as integer (NONE: 0, ERROR: 1, WARNING: 2, INFO: 3, DEBUG: 4, default: 3)
```

> @@ -141,6 +141,12 @@ ifeq ($(DEVELHELP),1)
   CFLAGS += -DDEVELHELP
 endif
 
+# If not already set in CFLAGS, set logging level by default to INFO, e.g. 3.
+LOG_LEVEL ?= 3

Why not leave it unset and check if it is set as default? This way in case we want to change the default we don't have to do it in three places (here, the doc in `vars.inc.mk`, and the define) and confusions for a person trying to change just [the define](https://github.com/RIOT-OS/RIOT/blob/1dc479de00ed81981d7495e6e3c53b0a791b3efb/core/include/log.h#L70) are avoided.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11592#pullrequestreview-242555069
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190528/befa717e/attachment.html>


More information about the notifications mailing list