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

refactor: refactored platform assignment into get_platform function #29971

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

Conversation

iw4p
Copy link

@iw4p iw4p commented Apr 26, 2024

Improved code readability and maintainability by moving platform identifiers into a dictionary.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 26, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

return target

print(f'Not sure which binary to download for {host}')
return None
Copy link
Member

Choose a reason for hiding this comment

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

Not sure. Now there are two functions and more lines of code, so this is harder to read and to maintain

@laanwj
Copy link
Member

laanwj commented Apr 26, 2024

i understand you're only trying to help, but please don't flood the project with small Python script refactorings. We're severely bottlenecked on review, and tend to have a philosophy to not fix what isn't broken.

@iw4p
Copy link
Author

iw4p commented Apr 26, 2024

I understood but if the philosophy is to not fix what isn't broken, so what "refactor" (for structural changes that do not change behavior) means? I am really trying to start reading the code from easy parts to contribute and refactor codes and update them. Some codes are for more than 4 years ago which are using subprocess for a single curl refrence which is not optimized when python's providing requests built-in library.

@laanwj
Copy link
Member

laanwj commented Apr 26, 2024

Sure, but for example: if you're going to do things like optimization, consider if speed is important in the process at that point (in many cases it's not, that's why the tool is in Python and not C++). If it is important, we'll also need to see benchmarks that the speed is noticibly faster.

If you've been involved for a while and know where the pain points are, this is easier to know. For new contributors I wouldn't really recommend doing refactors at all.

If you're looking for something to work on (which is great!), please check the good first issue label.

@iw4p
Copy link
Author

iw4p commented Apr 26, 2024

I appreciate your response and explanation. I'll take a quick look into it!

@maflcko
Copy link
Member

maflcko commented Apr 26, 2024

Also:

Getting started to contribute to Bitcoin Core

Setting up your development environment

New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and functional tests. Once that is done, you are all set.

If you need more help getting started, please refer to the following resources:

Pick something to work on

If you are looking for useful contributions to help out with, you can

  • Spend time on in-depth review of open pull requests and compare your review with the review done by others. This may be time-consuming, but it will teach you relevant skills for this project and for yourself to apply elsewhere. Moreover, on some pull requests there remain small follow-ups that were found during review, so you'll naturally find stuff to work on if you start out by reviewing pull requests done by others.
  • Search through the good first issues or the ones that are up for grabs. Some of them might no longer be applicable. So if you are interested, but unsure, you might want to leave a comment on the issue first.
  • Write tests to improve the coverage. Any kind of test is welcome and coverage information can be obtained from a relatively recent coverage report. If you are unsure, don't hesitate to check back first.
  • Help with review and testing. There are easy ones such as the gui and rpc. However, review on any open pull request is welcome. Review will also help you understand the codebase better.
  • Help on meta projects related to Bitcoin Core, such as https://github.com/corecheck.
  • Join the Bitcoin Core IRC channel(s) and leave a comment to share what you are interested in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants