Skip to content

Improve log style consistency#148

Merged
Siegrift merged 3 commits intomainfrom
log-style
Dec 4, 2023
Merged

Improve log style consistency#148
Siegrift merged 3 commits intomainfrom
log-style

Conversation

@Siegrift
Copy link
Collaborator

@Siegrift Siegrift commented Dec 1, 2023

Closes #144

@Siegrift Siegrift requested a review from andreogle December 1, 2023 09:37
@Siegrift Siegrift self-assigned this Dec 1, 2023

// Intentionally making the message as constant so that it is not accidentally changed. API integrations team (and maybe
// other teams, such as monitoring) will listen for this exact message to parse the heartbeat.
const HEARTBEAT_LOG_MESSAGE = 'Sending heartbeat log.';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc: @bdrhn9, @metobom I've changed the log message (there is not a dot at the end).

Copy link
Member

@andreogle andreogle left a comment

Choose a reason for hiding this comment

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

Kinda puzzled by this one. What's the reason for the full stops everywhere?

Most log message (I think?) I've seen don't have a full stop at the end. It seems kinda unnecessary imo - we're not exactly writing long paragraphs in our log messages. That said, I don't have a big problem with it

@Siegrift
Copy link
Collaborator Author

Siegrift commented Dec 1, 2023

Kinda puzzled by this one. What's the reason for the full stops everywhere?

I dislike the inconsistency between some messages having it and some don't. We could do with or without a dot and I chose to add it because sometimes you want to write two sentences and use a dot in the middle. E.g. Some error happened. Skipping update.. It feels wierd where is no dot at the end but there is one in the middle.

@bbenligiray
Copy link
Member

I omit the period if I'm writing a single sentence and use it otherwise. It's more of a style thing though, I find single sentences ending with periods too sassy.

Base automatically changed from pusher-to-airnode-feed to main December 1, 2023 13:47
@Siegrift
Copy link
Collaborator Author

Siegrift commented Dec 1, 2023

Yeah, it's mostly matter of style and it's not a big deal if it's inconsistent, but I still consider it an improvement. I did a quick search and there are people recommending the same log style.

@Siegrift Siegrift merged commit fe01bfc into main Dec 4, 2023
@Siegrift Siegrift deleted the log-style branch December 4, 2023 12:48
@Siegrift
Copy link
Collaborator Author

Siegrift commented Dec 4, 2023

Sorry, after I merged this PR I realized we might have not finished the discussion. Let me know if you strongly see it your way and I can revert. Otherwise, I'll keep it merged.

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.

Improve consistency of log statements

3 participants