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

To nest or not to nest? #1819

Closed
nopara73 opened this issue Jul 7, 2019 · 10 comments

Comments

@nopara73
Copy link
Collaborator

commented Jul 7, 2019

#1701, #1724, #1730, #1735, #1734, #1731, #1753, #1727, #1726, #1732, #1729, #1728, #1806, #1764 opened by @yahiheb to increase nesting.

I think this should be organic, not automatic. The developer should make the decision for code readability reasons. So I propose closing these pull requests. Thoughts?

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

@molnard is on a holiday, I know he especially prefers not nesting, sometimes to an unhealthy extent. @lontivero @danwalmsley @jmacato ?

@jmacato

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

In my opinion, minimal nesting (like less than 2 levels of brackets) is fine but in all case we should have a project-level consensus on these kinds of code readability issues so that we avoid friction in the future.

@yahiheb

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

@nopara73 Some of the code in these pull requests improve the code performance (not code readability).
If you guys decide to close these PRs should I open new PR for only the code that improves the efficiency?

@danwalmsley

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

@nopara73 I think we need to discuss this in the meeting tomorrow.

@yahiheb

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

Is the meeting open for anyone to join? and at what time?

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

@yahiheb It's for anyone. Please use this slack link, this is where the google hangouts link will be published in the #wasabi-dev channel. Also note that our meetings are livestreamed, then immediately unpublished and some snippets are cut out of them for the youtube channel. So if that's good for you.

https://join.slack.com/t/tumblebit/shared_invite/enQtNjQ1MTQ2NzQ1ODI0LWIzOTg5YTM3YmNkOTg1NjZmZTQ3NmM1OTAzYmQyYzk1M2M0MTdlZDk2OTQwNzFiNTg1ZmExNzM0NjgzY2M0Yzg

We have a dev meeting every Monday, 6:00 PM, Coordinated Universal Time (UTC)

Regarding the slack link, I've seen people having trouble getting into some slack channels when they join through links. In if you don't get to the dev room, ping @lontivero and @nopara73 on slack to make sure you get the link. (Both of us so one of us will surely won't miss it.)

@yahiheb

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

Thank you, I will try to join the meeting if I can.

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

I have review the PRs again and approved those that IMO make the code more clear.

@yahiheb

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

@nopara73 I think #1724, #1764 are worth taking a look at.
I also moved the code that improves the performance in #1859, #1860.
#1701, #1730, #1735, #1734, #1731, #1727, #1726, #1732, #1729, #1728 have some if statements that can be inverted for better readability without adding any nesting, should they be inverted? If not then all those PRs can be closed.
I closed #1753.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 9, 2019

As we decided in the meting, these PRs will be judged on individual basis, so I'm closing this issue.

@nopara73 nopara73 closed this Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.