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
Advertise SCRAM / SASL support in addition to PLAIN #4113
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4113 +/- ##
==========================================
- Coverage 74.31% 73.78% -0.53%
==========================================
Files 353 353
Lines 22738 22789 +51
==========================================
- Hits 16897 16816 -81
- Misses 4552 4693 +141
+ Partials 1289 1280 -9
... and 12 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment 🤗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are three quirks that I pointed out many times over multiple PRs:
- The usage of
else
when the code without it would be easier to understand (by having fewer branches). That's a generalization of https://go.dev/wiki/CodeReviewComments#indent-error-flow that is already mentioned in CONTRIBUTING.md, but we probably could add to the list of our idiosyncrasies. - The usage of
assert
instead ofrequire
even if the test can't continue. That small detail becomes pretty important when we add another backend – the test process crashes instead of just marking a test as a failure. - The creation of new test helpers. I don't think the more, the merrier rule applies there – the list of existing helpers one has to check to decide if they should be used or not should be smaller, not larger, especially if some of them are used only once.
So please be aware of those things and try not to repeat them again.
@FerretDB/core, please don't ignore those things in your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Closes #3778.
Readiness checklist
task all
, and it passed.@FerretDB/core
), Milestone (Next
), Labels, Project and project's Sprint fields.