-
Notifications
You must be signed in to change notification settings - Fork 158
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
Minor Seq Optimizations #922
Conversation
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.
While I appreciate this PR, the proper thing to do here is most likely a bump of the analyzer tool and packages.
What you detected manually should be caught be a tool.
|> Seq.filter (linkNotDefined doc) | ||
|> Seq.map (getTypeLink ctx) | ||
|> Seq.choose (fun line -> | ||
if linkNotDefined doc line then |
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.
Please invert the if/else here, I prefer the negative branch to be the if branch.
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 updated to use a local variable to prevent a double negative. Let me know if you prefer to not use 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 feel like updating the definition of linkNotDefined
to linkDefined
might also be an option. It is only used once and could probably ne simplified to a List.exist
instead of the map/reduce combo.
Thanks for the quick review! I will go ahead and make these changes when I get back from travel early next week |
Thank you, safe travels! |
This codebase is rather old and has many authors. I don't believe there is a good reason these cases are not combined. Could you address these as well? |
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.
Thanks!
I was poking around the repo and saw these few spots where I thought we could get some minor performance gains. There are also quite a few areas where we have multiple
Seq.Filter
's next to each other. I would also like to suggest updating those unless maintainers know of a reason why we have multiple filter iterations next to each other. Hope this helps!