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

Extract common parsing logic into single function. #1394

Merged
merged 4 commits into from
Dec 7, 2022

Conversation

HoytRen
Copy link
Contributor

@HoytRen HoytRen commented Nov 24, 2022

Closes #1331

When I compile cel-key, I see the binary is output to root, then I guess 'celestia' in the .igitigonre means the compiled binary, however it's moved to another place and not in root now.

@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2022

Codecov Report

Merging #1394 (cf8760f) into main (da4f54b) will increase coverage by 0.44%.
The diff coverage is 55.10%.

@@            Coverage Diff             @@
##             main    #1394      +/-   ##
==========================================
+ Coverage   56.02%   56.46%   +0.44%     
==========================================
  Files         188      189       +1     
  Lines       11551    11477      -74     
==========================================
+ Hits         6471     6481      +10     
+ Misses       4443     4360      -83     
+ Partials      637      636       -1     
Impacted Files Coverage Δ
cmd/celestia/full.go 90.00% <0.00%> (+51.97%) ⬆️
cmd/celestia/light.go 90.00% <0.00%> (+52.50%) ⬆️
cmd/celestia/util.go 56.52% <56.52%> (ø)
cmd/celestia/bridge.go 100.00% <100.00%> (+24.59%) ⬆️
share/ipld/get.go 93.41% <0.00%> (+1.23%) ⬆️
header/core/listener.go 58.49% <0.00%> (+5.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@HoytRen
Copy link
Contributor Author

HoytRen commented Nov 24, 2022

The errors seem not on my side?

@renaynay renaynay added kind:misc Attached to miscellaneous PRs area:config CLI and config labels Nov 25, 2022
Copy link
Member

@renaynay renaynay 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 this PR @HoytRen !

Two requests:

  • please remove the //nolint:dupl directive at the top of cmd/celestia/full.go and cmd/celestia/light.go
  • please remove the change made to the .gitignore file

@HoytRen
Copy link
Contributor Author

HoytRen commented Dec 1, 2022

Thanks for your guide. The lint problem is gone. I will leave the problem cause by .gitignore file by now, and let other people fix it later.

@HoytRen HoytRen requested a review from renaynay December 1, 2022 13:36
Wondertan
Wondertan previously approved these changes Dec 5, 2022
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Thanks and LGTM. Care for a .gitingore fix in the separate PR?

@HoytRen
Copy link
Contributor Author

HoytRen commented Dec 6, 2022

Sure, should I create another issue about the .gitignore first, or simply submit a PR? and I guess it's better to fix all detected problems once?

@Wondertan
Copy link
Member

Yeah, let's also make an issue with the problem statement.

@Wondertan Wondertan merged commit d1144b6 into celestiaorg:main Dec 7, 2022
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Dec 9, 2022
@HoytRen HoytRen deleted the #1331 branch March 21, 2023 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config CLI and config kind:misc Attached to miscellaneous PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmd/celestia: Extract common parsing logic into single function
4 participants