Skip to content

Conversation

@mattbaker-digital
Copy link

ARDUINOLOG_DISABLE_LINE_END_DEFINES

Fixes #2

ARDUINOLOG_DISABLE_LINE_END_DEFINES

Fixes JSC-TechMinds#2
This overcomes problems if ARDUINOLOG_DISABLE_LINE_END_DEFINES is defined.
@vzahradnik
Copy link
Member

I don't like that you are redefining one particular flag. Do you think that we could get rid of the defines completely and instead add them into the Logging class? This approach seems more clear to me and shouldn't be complicated to implement.

@vzahradnik
Copy link
Member

Alternatively - and I think it's cleaner - just prepend a prefix, similar to how color is defined.

#define LOG_LINE_ENDING_CR "\n"
#define LOG_LINE_ENDING_LF "\r"
#define LOG_LINE_ENDING_NL "\n\r"

@mattbaker-digital
Copy link
Author

I don't like that you are redefining one particular flag. Do you think that we could get rid of the defines completely and instead add them into the Logging class? This approach seems more clear to me and shouldn't be complicated to implement.

This was mainly to keep backwards compatibility and prevent breaking existing code using it. I'd like to get rid of lots of the defines and clear some more up but wasn't sure how breaking I could make the changes. I'd also like to add the ability to daisy chain log objects with a module name similar to Python logging.

@vzahradnik
Copy link
Member

Take this as a fork which to some degree has open hands to do breaking changes. People can stay on the current release and if we introduce breaking changes, I will mention that in the release notes.

@vzahradnik
Copy link
Member

If you prepare a proper PR without defines, I will happily review it.

@mattbaker-digital
Copy link
Author

mattbaker-digital commented Sep 11, 2024

Alternatively - and I think it's cleaner - just prepend a prefix, similar to how color is defined.

#define LOG_LINE_ENDING_CR "\n"
#define LOG_LINE_ENDING_LF "\r"
#define LOG_LINE_ENDING_NL "\n\r"

The problem with this is the verbosity when used as intended in the examples. E.g. Log.error("%S Error with binary values : %b, %B"CR, PSTRPTR(LOG_AS), 23, 345808); and similar would need to be changed to Log.error("%S Error with binary values : %b, %B"LOG_LINE_ENDING_CR, PSTRPTR(LOG_AS), 23, 345808);

This may have been used before the introduction of the ln suffixed methods, in which case these could go altogether.

I also hate that the LF macro is actually ASCII CR and CR macro is actually ASCII LF. Not sure how this happened in the first place.

@mattbaker-digital
Copy link
Author

If you prepare a proper PR without defines, I will happily review it.

I'm working on and off this at the moment. Hopefully I'll be able to contribute something in the not too distant future.

@vzahradnik
Copy link
Member

That's OK. Besides this change, I have resolved your two other issues. Once this change gets merged, I will publish a new release so that it's easy to use.

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.

Increase uniqueness of CR LF NL defines (or remove) as these can cause conflicts with other libraries

3 participants