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.

Sign up for GitHub

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

Jump to bottom

refactor: using slices.Contains to simplify the code #1283

Merged
merged 1 commit into from
Jan 11, 2025
Merged

refactor: using slices.Contains to simplify the code #1283

merged 1 commit into from
Jan 11, 2025

Conversation

zhoufanjin
Copy link
Contributor

Using new function added in the go1.21 standard library, which can make the code more concise and easy to read.

@ffranr ffranr self-requested a review January 9, 2025 11:13
@ffranr ffranr requested review from bhandras and gijswijs and removed request for bhandras January 9, 2025 11:14
ffranr
ffranr requested changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

This isn't correct. return is missing.

@ffranr ffranr removed the request for review from gijswijs January 9, 2025 11:22
@zhoufanjin
refactor: using slices.Contains to simplify the code
6930554 
Signed-off-by: zhoufanjin <shuangcui@live.cn>
@zhoufanjin
Copy link
Contributor Author

This isn't correct. return is missing.

Sorry for my carelessness. Modified

bhandras
bhandras approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@coveralls
Copy link

coveralls commented Jan 9, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 12690734269

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 16 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.02%) to 40.954%
Files with Coverage Reduction New Missed Lines %
tappsbt/create.go 2 53.22%
tapgarden/planter.go 2 74.12%
asset/mock.go 3 92.39%
tapgarden/caretaker.go 4 68.5%
asset/asset.go 5 80.75%
Totals Coverage Status
Change from base Build 12650018423: 0.02%
Covered Lines: 26411
Relevant Lines: 64490
💛 - Coveralls

@zhoufanjin
Copy link
Contributor Author

LGTM 🎉

Thanks for your review!

dstadulis
dstadulis approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@dstadulis dstadulis left a comment

Choose a reason for hiding this comment

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

ty for the simplification

itest/loadtest/load_test.go
}

return false
return slices.Contains(configuredCases, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@dstadulis
Copy link
Contributor

CI faults due to repo's CI using outdated branch. Fix posted here: #1291 can merge #1283 after successful CI / merge on #1291

@bhandras bhandras requested a review from ffranr January 11, 2025 08:50
ffranr
ffranr approved these changes Jan 11, 2025
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! ❤️

@ffranr ffranr added this pull request to the merge queue Jan 11, 2025
Merged via the queue into lightninglabs:main with commit e56f2fd Jan 11, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffranr ffranr ffranr approved these changes

@bhandras bhandras bhandras approved these changes

@dstadulis dstadulis dstadulis approved these changes

Assignees

@zhoufanjin zhoufanjin

Labels
None yet
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zhoufanjin @coveralls @dstadulis @bhandras @ffranr