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

Add findandmodify to the common.Commands map #1730

Merged
merged 9 commits into from
Jan 3, 2023
Merged

Add findandmodify to the common.Commands map #1730

merged 9 commits into from
Jan 3, 2023

Conversation

b1ron
Copy link
Contributor

@b1ron b1ron commented Dec 30, 2022

Description

Closes #1727.

Readiness checklist

  • I added unit tests for new functionality or bug fixes.
  • I added integration tests for new functionality or bug fixes.
  • I added compatibility tests for new functionality or bug fixes.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I added/updated comments for both exported and unexported top-level declarations (functions, types, etc).
  • I checked comments rendering with task godocs.
  • I ensured that the title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Assignee, Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@vercel
Copy link

vercel bot commented Dec 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
ferret-db ✅ Ready (Inspect) Visit Preview Jan 2, 2023 at 0:48AM (UTC)

@vercel vercel bot temporarily deployed to Preview December 30, 2022 14:50 Inactive
@b1ron b1ron self-assigned this Dec 30, 2022
@b1ron b1ron added the code/enhancement Some user-visible feature could work better label Dec 30, 2022
@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Merging #1730 (29cbfc8) into main (c323091) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1730      +/-   ##
==========================================
- Coverage   69.07%   69.01%   -0.07%     
==========================================
  Files         301      301              
  Lines       14297    14303       +6     
==========================================
- Hits         9876     9871       -5     
- Misses       3493     3496       +3     
- Partials      928      936       +8     
Impacted Files Coverage Δ
internal/handlers/common/msg_listcommands.go 100.00% <100.00%> (ø)
internal/handlers/pg/pgdb/collections.go 64.56% <0.00%> (-4.73%) ⬇️
build/version/version.go 63.01% <0.00%> (-4.11%) ⬇️
internal/handlers/pg/pgdb/database_metadata.go 81.92% <0.00%> (-3.62%) ⬇️
internal/clientconn/conn.go 44.34% <0.00%> (-2.61%) ⬇️
internal/clientconn/listener.go 77.84% <0.00%> (-1.71%) ⬇️
internal/handlers/common/filter.go 84.86% <0.00%> (ø)
internal/handlers/tigris/tigrisdb/collections.go 66.66% <0.00%> (+4.76%) ⬆️
internal/handlers/tigris/msg_create.go 74.64% <0.00%> (+7.04%) ⬆️
... and 1 more
Flag Coverage Δ
integration 64.40% <100.00%> (+<0.01%) ⬆️
mongodb 5.97% <0.00%> (-0.01%) ⬇️
pg 52.98% <100.00%> (-0.09%) ⬇️
tigris 38.92% <100.00%> (+0.11%) ⬆️
unit 28.95% <0.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@b1ron b1ron marked this pull request as ready for review December 30, 2022 15:36
@b1ron b1ron requested a review from a team as a code owner December 30, 2022 15:36
@vercel vercel bot temporarily deployed to Preview December 30, 2022 15:37 Inactive
@b1ron b1ron requested review from a team and noisersup and removed request for a team December 30, 2022 15:38
noisersup
noisersup previously approved these changes Dec 30, 2022
Copy link
Member

@noisersup noisersup left a comment

Choose a reason for hiding this comment

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

LGTM!

internal/handlers/common/msg_listcommands.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview December 30, 2022 18:22 Inactive
@b1ron b1ron requested a review from noisersup December 30, 2022 18:24
@b1ron
Copy link
Contributor Author

b1ron commented Dec 30, 2022

@noisersup I pushed another commit to clean up a bit. PTAL.

@vercel vercel bot temporarily deployed to Preview December 31, 2022 13:13 Inactive
Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

Let's also update TestCommandsDiagnosticListCommands

internal/handlers/common/msg_listcommands.go Outdated Show resolved Hide resolved
internal/handlers/common/msg_listcommands.go Outdated Show resolved Hide resolved
internal/handlers/common/msg_listcommands.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview January 2, 2023 10:15 Inactive
@b1ron b1ron requested a review from AlekSi January 2, 2023 11:22
@vercel vercel bot temporarily deployed to Preview January 2, 2023 12:20 Inactive
rumyantseva
rumyantseva previously approved these changes Jan 2, 2023
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

So, in this solution, we use a separate ignoreLowercaseCmds map to store "ignored" values. I like it because it stores the exact things we don't want to show (lower-cased commands). On the other hand, if we have more duplicates, we must remember to add them to ignoreLowercaseCmds too.

Another potential way would be to iterate through Commands, store unique commands in a map and check for duplicates (for example, like we did here - https://github.com/FerretDB/FerretDB/blob/main/internal/types/document_validation.go#L71).

But I like your solution more because with your solution we don't need to take care of letters lower/upper case etc.

@rumyantseva
Copy link
Contributor

@b1ron linter complains a little bit because empty structs in your map can be set simpler (just {} instead of struct{}{}), please take a look.

@b1ron
Copy link
Contributor Author

b1ron commented Jan 2, 2023

@b1ron linter complains a little bit because empty structs in your map can be set simpler (just {} instead of struct{}{}), please take a look.

Yes will fix that. Thanks!

@vercel vercel bot temporarily deployed to Preview January 2, 2023 12:46 Inactive
@vercel vercel bot temporarily deployed to Preview January 2, 2023 12:48 Inactive
@b1ron b1ron enabled auto-merge (squash) January 2, 2023 13:32
@b1ron b1ron requested a review from rumyantseva January 2, 2023 13:39
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM

@b1ron b1ron changed the title Add findandmodify to MsgListCommands Add findandmodify to the common.Commands map Jan 2, 2023
@b1ron b1ron merged commit a6f86f6 into FerretDB:main Jan 3, 2023
@AlekSi AlekSi added this to the v0.8.1 milestone Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/enhancement Some user-visible feature could work better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle the findAndModify legacy shell method
4 participants