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

chaincfg: split params into per-network files. #1265

Merged
merged 1 commit into from
Jun 6, 2018

Conversation

JFixby
Copy link
Contributor

@JFixby JFixby commented Jun 5, 2018

The purpose of this PR is to reduce the number of code lines in the chaincfg/params.go (from 1262 to 650) improving its readability for future maintenance.

Changes:

  • move variable MainNetParams into dedicated file mainnetparams.go
  • move variable TestNetParams into dedicated file testnetparams.go
  • move variable SimNetParams into dedicated file simnetparams.go

No logical changes were made. Just Cut&Paste.

@alexlyp
Copy link
Member

alexlyp commented Jun 5, 2018

nack

Copy link
Member

@davecgh davecgh 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 the PR. The change seems reasonable, however, the commit message and PR description do not conform to the Code Contribution Guidelines, most notably the Model Git Commit Messages.

Please take care of that and then I'll review the diff to ensure there are no other changes in the params.

@JFixby JFixby changed the title brush up chaincfg/params.go reduce chaincfg/params.go by splitting into multiple files Jun 5, 2018
@JFixby JFixby changed the title reduce chaincfg/params.go by splitting into multiple files reduce chaincfg/params.go by splitting into multiple files Jun 5, 2018
@JFixby JFixby changed the title reduce chaincfg/params.go by splitting into multiple files chaincfg: reduce params.go by splitting into multiple files Jun 5, 2018
@JFixby JFixby force-pushed the brushup-chaincfg branch 2 times, most recently from 89d5b73 to 7031cf8 Compare June 5, 2018 17:09
@JFixby
Copy link
Contributor Author

JFixby commented Jun 5, 2018

Thank you @davecgh

Should be good now.
Let me know if I still need to fix something.

@davecgh davecgh changed the title chaincfg: reduce params.go by splitting into multiple files chaincfg: split params into per-network files. Jun 5, 2018
@davecgh
Copy link
Member

davecgh commented Jun 5, 2018

The commit title still exceeds the max of 50 chars. I changed the PR title to one that fits. I'll still need to review the code to ensure no changes made it in.

The purpose of this PR is to reduce the number of code lines in the
chaincfg/params.go (from 1262 to 650) improving its readability for
future maintenance.

Changes:
 - move variable `MainNetParams` into dedicated file `mainnetparams.go`
 - move variable `TestNetParams` into dedicated file `testnetparams.go`
 - move variable `SimNetParams` into dedicated file `simnetparams.go`

No logical (functional) changes were made. Just Cut&Paste.
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

In addition to manual review, I used the following process to ensure none of the values change:

$ git checkout master
$ cat params.go | sed -ne '/\/\/ Copyright/,/^package chaincfg/p' > mainnetparams2.go; cat params.go | sed -ne '/\/\/ MainNetParams defines the/,/^}/p' >> mainnetparams2.go
$ cat params.go | sed -ne '/\/\/ Copyright/,/^package chaincfg/p' > testnetparams2.go; cat params.go | sed -ne '/\/\/ TestNet2Params defines the/,/^}/p' >> testnetparams2.go
$ cat params.go | sed -ne '/\/\/ Copyright/,/^package chaincfg/p' > simnetparams2.go; cat params.go | sed -ne '/\/\/ SimNetParams defines the/,/^}/p' >> simnetparams2.go

$ goimports -w *params2.go

$ git checkout JFixby-brushup-chaincfg
Switched to branch 'JFixby-brushup-chaincfg'

$ openssl sha256 mainnetparams.go mainnetparams2.go
SHA256(mainnetparams.go)= 49c7d02521af618882c4972cccd39329497095ebe3dadc337e92336218a5eb41
SHA256(mainnetparams2.go)= 49c7d02521af618882c4972cccd39329497095ebe3dadc337e92336218a5eb41

$ openssl sha256 testnetparams.go testnetparams2.go
SHA256(testnetparams.go)= c364988f55d0ff424e135dac760a60bdfd99c784f9f8587a279d9d07e1e61fcd
SHA256(testnetparams2.go)= c364988f55d0ff424e135dac760a60bdfd99c784f9f8587a279d9d07e1e61fcd

$ openssl sha256 simnetparams.go simnetparams2.go
SHA256(simnetparams.go)= 4e46360de44a97dd637d58e5a198e3759346a2d62fee6b697b6ee9fa0e2a79ed
SHA256(simnetparams2.go)= 4e46360de44a97dd637d58e5a198e3759346a2d62fee6b697b6ee9fa0e2a79ed

$ rm *params2.go

@davecgh davecgh merged commit d454c9e into decred:master Jun 6, 2018
@davecgh davecgh added this to the 1.3.0 milestone Jun 7, 2018
@jrick
Copy link
Member

jrick commented Jun 19, 2018

I'm finding this change quite annoying. I was previously able to use go-to-reference in my editor to open the parameters file and then search that exact same file for the particular network's parameters I was interested in. After this change, the actual parameter values are in different files and it takes more effort to find the correct file and parameter definitions.

@JFixby
Copy link
Contributor Author

JFixby commented Jun 19, 2018

Is it about the "Find all references"?

image

@jrick
Copy link
Member

jrick commented Jun 19, 2018

No, i'm talking about go to definition on fields of a params object, not find all references of the defined field. The latter takes exuberantly long (and it's even faster to find and open the files themselves) because the tooling has to search for any references in my entire GOPATH.

@JFixby JFixby deleted the brushup-chaincfg branch June 19, 2018 21:25
@JFixby
Copy link
Contributor Author

JFixby commented Jun 20, 2018

Generally, it is not easy to read a file with 1262 lines of code.
Hard to read correlates with hard to maintain and hard to spot a problem.
Longer files = more bugs. Shorter files = less bugs.

As far as I understand, your case is the following:
You are somewhere in the code and you are looking for the network parameter value assignment.

I may suggest the following solution:

  1. click "Jump to declaration/Go to definition" (Ctrl + mouse click)
  2. select "Find all references/occurrences/usages" - there you find the assignment and that's it

However, I found that in VSCode IDE it is not that easy.

  1. "Go to definition" takes 3-5 seconds, which is abnormally slow. Eclipse IDE was doing it instantly in 2005.
  2. "Find all references" takes about 30-60 seconds to perform.

I checked VSCode issue tracker. Both these issues are known and were reported multiple times in there. It seems nobody is interested in fixing it. VSCode devs just keep closing the tickets.

I checked GoLanD. There I found "Find all references"-feature that does exactly what you need. It instantly returns detailed search results, that will bring you to the exact line of code you look for.

image

@chappjc
Copy link
Member

chappjc commented Jun 21, 2018 via email

@JFixby
Copy link
Contributor Author

JFixby commented Jun 21, 2018

Good. But opening the right file in the first step is not smooth. You still have to call the CTRL-F to find the code line you need (value assignment) and scroll over 1200 lines up and down, checking you are actually looking at the right network params set, because there are 3 networks in the same file, it is easy to mix up.

Btw, GoLanD will aid you well. In one single move it will point you directly from usage code to the assignment. After this PR you just need to glance at the file name: is it the mainnetparams.go or the testnetparams.go (see screenshot above)

JFixby pushed a commit to JFixby/dcrd that referenced this pull request Oct 21, 2018
blockchain/indexers: fix bug in indexer re-org catch up
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