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

Clean code and fix issues reported by staticcheck #223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sarroutbi
Copy link

No description provided.

@aviary3-wk
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@sarroutbi sarroutbi force-pushed the 202305251644_clean_code branch 4 times, most recently from 498236a to e5b6f40 Compare May 26, 2023 11:04
@sarroutbi sarroutbi marked this pull request as ready for review May 26, 2023 11:12
@sarroutbi
Copy link
Author

sarroutbi commented Jun 7, 2023

Any chance this PR could be reviewed?

@Florimond
Copy link

@sarroutbi Wondering the same about my own PR. The project seems pretty dead to me.

dustinhiatt-wf
dustinhiatt-wf previously approved these changes Sep 27, 2023
Copy link
Contributor

@matthinrichsen-wf matthinrichsen-wf left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to clean these up! I just had a few nits, otherwise looks great.

@@ -412,7 +412,7 @@ func TestNodeSplit(t *testing.T) {

rt, err = mutable.Commit()
require.NoError(t, err)
rt, err = Load(cfg.Persister, rt.ID(), comparator)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we require.NoError here instead of underscoring?

@@ -504,7 +504,7 @@ func TestRandom(t *testing.T) {

require.NoError(t, err)
expected := toOrdered(items).toItems()
result, err := mutable.(*Tr).toList(itemsToValues(expected...)...)
result, _ := mutable.(*Tr).toList(itemsToValues(expected...)...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here:

Can we require.NoError here instead of underscoring?

@@ -762,7 +762,7 @@ func TestSecondCommitMultipleSplits(t *testing.T) {
mutable := rt.AsMutable()
mutable.AddItems(items[:25]...)
mutable.(*Tr).verify(mutable.(*Tr).Root, t)
rt, err := mutable.Commit()
rt, _ = mutable.Commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well:

Can we require.NoError here instead of underscoring?

Signed-off-by: Sergio Arroutbi <sarroutb@redhat.com>
@sarroutbi
Copy link
Author

Hello @matthinrichsen-wf . Can you please check, when possible, if this PR can be merged after your recommendations?

Thanks !!!

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