-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
1 changed file
with
34 additions
and
0 deletions.
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
2475df9
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.
Hi @frittex14 ,
I'm learning Rust. I like your new "beginner friendly" function -- I agree your function is simpler than the existing function :
Result
; your function unwraps the result, whereas the existing function propagates errorsString
, whereas the existing function expects anything that implementsAsRef<Path>
But you say your "beginner friendly" function is not efficient , why not?
I think your function uses a
BufReader
and theBufReader.lines()
method... I think both functions would be equally efficient?2475df9
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.
Hey!
Actually, I've already covered this topic in issue #1677, but I understand your confusion. I've had a few people ask me about this before, so I'm definitely open to making changes. If you have any ideas on how to clarify this, feel free to open a PR. I've already proposed a change in the mentioned issue, so you can use that, or come up with something yourself. Either way, thank you for mentioning my slip.
Let me know if you have any other questions or concerns. Cheers!
2475df9
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 @frittex14 ,
I like your suggested change in #1677 , mentioning "idiomatic" instead of "efficient".
Issue #1677 is helpful, as @woofyzhao says, this line is confusing.
"String in memory" suggests the "Beginner friendly" code creates a string in memory of the entire file contents.
EDIT Now I see how "This process is more efficient than creating a
String
in memory"... was there before your change!It seems like it refers to your change; comparing "Beginner-friendly" to "Efficient". ... Instead, it meant to refer to an approach not shown at all!
It also looks like this pull request #1681 (from @arthurmilliken ) tries to address the issue
#1681 changes the "Beginner-friendly" method to make it less efficient and actually uses
read_to_string()
to "create a string in memory" But in my opinion, the code gets less beginner friendly!So I wonder about removing mention of "efficiency" altogether, just focus on "beginner-friendly" / "simpler"
Anyway , thanks for acknowledging... If I can think of a better solution I'll try to make a PR (would be my first)!
2475df9
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 actually never noticed the #1681 PR. In my opinion, it tries to fix the issue from the wrong side, unnecessarily making it more difficult than it really is. But who am I to judge?
Also, don't hesitate or be afraid to make a pull request. We're a really friendly community, and no one will "laugh" at you for your proposals. Open as many PRs/issues as you can, and let's improve together! Thanks!
2475df9
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.
OK, last comment, I learned there is pull request #1679 which I think does a good job of addressing my confusion.
Thanks again,