-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix: Under review tab pagination bug. #2552
Merged
lukebp
merged 6 commits into
decred:master
from
vibros68:2530_under_review_pagination_bug
Sep 3, 2021
Merged
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
12f1c4d
fix under review tab pagination bug: fetch 5 proposals per request ti…
vibros68 2d11125
add condition variable, fix scanNextStatusTokens function bug, remove…
vibros68 ad10ad3
reproduce scan tokens logic
vibros68 e70c680
batch pagination testing
vibros68 61a94d2
fix e2e test: records and inventory pagination
vibros68 2636412
clean up code
vibros68 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you missed my point. The purpose of splitting it into multiple variables is to improve readability, not just create a variable. See,
scanTokensCondition
brings no value to the code context. It's a condition, but it's a little hard to understand what does that condition mean, and specially, in reviews, it's hard to tell whether this is true or false just by reading the code. This is the "readability" I was talking about.I'd suggest this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your comment and suggestion here, but I really don't like that blog post.
The points his boss makes are the same design principles behind golang's design and why golang is so popular. Maybe the style he advocates for works when your entire stack is a single language, but that's not the case for us. Decred is mostly golang repos and golang uses the style that he is arguing against. I don't want developers to have to change their programming style when switching between the frontend and backend.
Don’t talk down on other developers by writing simplistic code
I disagree with this as well. Simple, clean code that doesn't make the reader think is easier to maintain, easier to extend, and easier to teach to somone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That article wasn't a good option, I misunderstood his point and realized after your comment. Removing it right now. I'm sorry.
But I personally think frontend approach is different than backend. JS allows many paradigms to get a cleaner code, and in terms of frontend development, it's harder to test edge-cases, specially when dealing with a non-typed language. Golang provides a strong typing, and indeed prevents many cases that JS don't. When we are talking about JS, you can compare and attribute almost any value to a variable, so that's why I opt to go for a more declarative and readable code. You can mess up a JS code completely just by passing an object where strings are expected, and many times, the error is shown at run-time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. The main point I wanted to get across is that, in general, readability should be valued over elegence.
Think about reading a book that takes complicated topics and writes about them in a ELI5 (explain it to me like I'm 5) style. That's how I want my code to read. If someone has to stop and think "what is this doing" or "why is it doing this" then it's probably not readable enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point. But this case includes next inventory page and next vote status. So if follow your style it should be
canScanNextInventoryPageOrVoteStatus
. @victorgcramos how do you thinks?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use
needsNextBatch
, it's good enough :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that
readability > elegance
. Sometimes a more imperative approach is easier to grasp than a declarative one and vice-versa. JS is very flexible and is the programmer's job to choose the best approach for each scenario.For example, see the implementation of a function that takes an array and returns an array with only odd numbers:
Declarative approach:
Imperative approach (using the
isOdd
util defined above to be more fair):In this case, the declarative approach reads way better. But now, take a look at this function we use on our code to generate a random color:
This is imperative code, right? How would we rewrite this using a declarative approach? Maybe something like:
Which one reads better? I'd go with the imperative code in this case.
So, for declarative vs imperative the answer is: it depends. Always ask yourself if a new programmer would understand your code. If not, try to simplify it. If you really can't do it, isolate the complicated code and add a good comment explaining it.
I didn't get the chance to read the article you guys are talking about (maybe @victorgcramos can add it back with a disclaimer?). But I strongly disagree with this too:
I think what @victorgcramos was trying to say is that most of the time, the declarative approach will be the right one because we are using React. React allows us to control data flow and state in a very declarative way and the benefit of this is that we don't get entangled in the implementation details. So, when using React we avoid doing things imperatively like directly manipulate the DOM and directly modify state. Very different than a lib that uses an imperative approach like jQuery, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great comments @tiagoalvesdulce. Thanks for taking the time to write that. I've saved this conversation to a gist for future reference.
https://gist.github.com/lukebp/790527592fba5987f2ec693216199766