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

Rename OP_* constants to OpCode* constants #620

Merged
merged 11 commits into from
May 26, 2022
Merged

Rename OP_* constants to OpCode* constants #620

merged 11 commits into from
May 26, 2022

Conversation

seeforschauer
Copy link
Contributor

No description provided.

@seeforschauer seeforschauer self-assigned this May 20, 2022
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #620 (c2ee663) into main (6f2ec6e) will not change coverage.
The diff coverage is 25.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #620   +/-   ##
=======================================
  Coverage   63.28%   63.28%           
=======================================
  Files         122      122           
  Lines        7157     7157           
=======================================
  Hits         4529     4529           
  Misses       2081     2081           
  Partials      547      547           
Impacted Files Coverage Δ
internal/wire/msg_header.go 69.04% <ø> (ø)
internal/wire/opcode_string.go 83.33% <ø> (ø)
internal/clientconn/conn.go 43.93% <22.72%> (ø)
internal/wire/msg_body.go 36.50% <30.00%> (ø)
Flag Coverage Δ
FerretDB 57.39% <21.87%> (ø)
MongoDB 6.29% <0.00%> (ø)
integration 57.42% <21.87%> (ø)
unit 25.09% <9.37%> (ø)

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

@seeforschauer seeforschauer marked this pull request as ready for review May 20, 2022 14:21
@seeforschauer seeforschauer requested review from a team and w84thesun and removed request for a team May 20, 2022 14:22
w84thesun
w84thesun previously approved these changes May 20, 2022
OP_KILL_CURSORS = OpCode(2007) // OP_KILL_CURSORS
OP_COMPRESSED = OpCode(2012) // OP_COMPRESSED
OP_MSG = OpCode(2013) // OP_MSG
OpCodeReply = OpCode(1) // OpCodeReply
Copy link
Member

Choose a reason for hiding this comment

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

We should not change string representations – they follow protocol spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@seeforschauer seeforschauer requested a review from AlekSi May 20, 2022 15:28
@AlekSi AlekSi requested review from a team and rumyantseva and removed request for a team May 23, 2022 05:42
rumyantseva
rumyantseva previously approved these changes May 23, 2022
Copy link
Member

@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

internal/wire/msg_header.go Outdated Show resolved Hide resolved
Co-authored-by: Elena Grahovac <elena@grahovac.me>
rumyantseva
rumyantseva previously approved these changes May 23, 2022
Copy link
Member

@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.

I see that codecov complains, but its complaints are related to the previous changes, not to the current ones. So, these complaints are irrelevant to the current purpose of the PR.

LGTM.

@AlekSi
Copy link
Member

AlekSi commented May 23, 2022

codecov/patch check is not required for that reason

@AlekSi AlekSi enabled auto-merge (squash) May 23, 2022 13:55
internal/wire/msg_header.go Outdated Show resolved Hide resolved
internal/wire/msg_header.go Outdated Show resolved Hide resolved
internal/wire/msg_header.go Outdated Show resolved Hide resolved
Co-authored-by: Alexey Palazhchenko <alexey.palazhchenko@gmail.com>
OP_KILL_CURSORS = OpCode(2007) // OP_KILL_CURSORS
OP_COMPRESSED = OpCode(2012) // OP_COMPRESSED
OP_MSG = OpCode(2013) // OP_MSG
// OpCodeReply is deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

We use it, so it should have a better description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

// OpCodeGetByOID is deprecated.
OpCodeGetByOID = OpCode(2003) // OP_GET_BY_OID

// OpCodeQuery is deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

We use it, so it should have a better description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@seeforschauer
Copy link
Contributor Author

added some comments, however, I acknowledge that it may differ how I understand the purpose from understanding of others.

@seeforschauer seeforschauer requested a review from AlekSi May 25, 2022 07:40
@AlekSi AlekSi disabled auto-merge May 26, 2022 13:57
@AlekSi AlekSi merged commit 61c844b into FerretDB:main May 26, 2022
@AlekSi AlekSi added the code/chore Code maintenance improvements label May 26, 2022
@AlekSi AlekSi added this to the v0.3.0 milestone May 26, 2022
@seeforschauer seeforschauer deleted the issue-587-revive branch May 27, 2022 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/chore Code maintenance improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants