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

[ZEND-517] Unnecessary log lines removal #559

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Conversation

titusen
Copy link
Contributor

@titusen titusen commented May 24, 2023

As above

@titusen titusen force-pushed the remove_unnecessary_logs branch 2 times, most recently from 232be79 to cac8a99 Compare May 24, 2023 11:13
Copy link
Contributor

@JackPiri JackPiri left a comment

Choose a reason for hiding this comment

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

Here it is better using already available function FormatVersion, since it makes this in a more concise way and it formats the build number as a suffix.
Please note FormatVersion should be fixed because it doesn't handle CLIENT_VERSION_BUILD=0 case and because it is bugged on beta builds (adding a +1).

@titusen
Copy link
Contributor Author

titusen commented May 26, 2023

Giacomo, I don't think this log line is not needed at all cause the full version with commit is printed here

LogPrintf("Horizen version %s (%s)\n", FormatFullVersion(), CLIENT_DATE);

src/metrics.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@JackPiri JackPiri left a comment

Choose a reason for hiding this comment

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

Left a couple of comments, nothing big but better to address in this PR.

@JackPiri
Copy link
Contributor

JackPiri commented May 26, 2023

Giacomo, I don't think this log line is not needed at all cause the full version with commit is printed here

LogPrintf("Horizen version %s (%s)\n", FormatFullVersion(), CLIENT_DATE);

Yes, I agree with you. Just remove one among:

  • LogPrintf("Horizen version %s (%s)\n", FormatFullVersion(), CLIENT_DATE);
  • LogPrintf("Zen version %s (%s)\n", FormatFullVersion(), CLIENT_DATE);

Copy link
Contributor

@JackPiri JackPiri left a comment

Choose a reason for hiding this comment

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

The PR is fine to me.
Thanks

Copy link
Contributor

@JackPiri JackPiri left a comment

Choose a reason for hiding this comment

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

Sorry I didn't notice the CI failure, BOOST_AUTO_TEST_CASE(test_FormatSubVersion) should be aligned accordingly.

Copy link
Contributor

@a-petrini a-petrini left a comment

Choose a reason for hiding this comment

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

PR looks good, thanks!

@titusen titusen force-pushed the remove_unnecessary_logs branch from f9ff4ab to 9f6867d Compare May 29, 2023 10:52
Copy link
Contributor

@drgora drgora left a comment

Choose a reason for hiding this comment

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

This is ok for me, thanks

@titusen titusen force-pushed the remove_unnecessary_logs branch 4 times, most recently from cf7a723 to 1b8d724 Compare May 30, 2023 13:03
src/metrics.cpp Outdated Show resolved Hide resolved
@titusen titusen force-pushed the remove_unnecessary_logs branch from 1b8d724 to 75b3ad2 Compare May 31, 2023 11:52
Client version print addition

Client version print format change

Unit test assertions addition
@titusen titusen force-pushed the remove_unnecessary_logs branch from 2644786 to 2baf037 Compare June 1, 2023 08:17
@ptagl ptagl merged commit 46b5320 into main Jun 2, 2023
@ptagl ptagl deleted the remove_unnecessary_logs branch June 2, 2023 08:00
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.

5 participants