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

Improved documentation #219

Merged
merged 12 commits into from Sep 7, 2019
Merged

Improved documentation #219

merged 12 commits into from Sep 7, 2019

Conversation

TimonPost
Copy link
Owner

@TimonPost TimonPost commented Jul 6, 2019

There are often questions around the problems people are facing because they don't know the fact that they should send packets at a consistent rate. And questions when to use our protocol. I hope that with this it is a bit more clear.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kstrafe
Copy link
Contributor

kstrafe commented Jul 6, 2019

I think a lot of this needs to be rewritten. The user only needs to know:

  • Packets are re-sent when you send a reliable packet
  • A connection times out if there has not been a packet from/to that connection in 5s (default), so you need to use heartbeats

@TimonPost
Copy link
Owner Author

TimonPost commented Jul 6, 2019

Kind of true, I think it is important to tell the exact purpose of where laminar is designed for. Every time we make a redit post, people will start asking the same questions and will refer to the same things. The same in our discord. There already have been 4 cases when people used laminar wrong. The same goes for the reliablity aspect of laminar. People somehow want to use laminar for file transfer.

There are two things we can do

  1. Make this more detailed and short
  2. Move some of this explanation to our book, which I think is more preferable than having all the text in the readme. And by doing that we keep the information in the readme short.

@jstnlef
Copy link
Contributor

jstnlef commented Jul 8, 2019

This needs a cargo fmt

@jstnlef
Copy link
Contributor

jstnlef commented Jul 8, 2019

I'm not entirely positive people will go look at the book to solve usage issues. (I know I haven't heh.) I think it would be worth putting the common issues someone would likely run into right in the README.

@TimonPost
Copy link
Owner Author

Alright, so we can agree that at least there must be info on common misinterpreted thinks about laminar. Then the question is, do we want it to be detailed, or do we want it to be very short, as @BourgondAries proposed?

@jstnlef
Copy link
Contributor

jstnlef commented Jul 9, 2019

I think it should be as detailed as it needs to be for people to but less likely to make the mistakes we've been seeing in the future.

@jstnlef
Copy link
Contributor

jstnlef commented Aug 19, 2019

Took a quick look at this and I have a few comments and more I'll flesh out into tomorrow. Mostly just some wording issues. Will post a proper review soon.

@TimonPost
Copy link
Owner Author

Is soon any time soon? Otherwise, I'll merge this and it can be done in the future. It is open for way to long meanwhile people might need it.

README.md Outdated Show resolved Hide resolved
docs/md_book/src/fragmentation.md Show resolved Hide resolved
docs/md_book/src/packet_header.md Show resolved Hide resolved
docs/md_book/src/protocols.md Show resolved Hide resolved
docs/md_book/src/intro.md Outdated Show resolved Hide resolved
docs/md_book/src/heartbeat.md Outdated Show resolved Hide resolved
docs/md_book/src/heartbeat.md Outdated Show resolved Hide resolved
docs/md_book/src/heartbeat.md Outdated Show resolved Hide resolved
docs/md_book/src/heartbeat.md Outdated Show resolved Hide resolved
docs/md_book/src/heartbeat.md Outdated Show resolved Hide resolved
Copy link
Owner Author

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

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

Worked through your comments, thanks for those!

Copy link
Contributor

@jstnlef jstnlef left a comment

Choose a reason for hiding this comment

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

Round 2

docs/md_book/src/heartbeat.md Outdated Show resolved Hide resolved
docs/md_book/src/intro.md Outdated Show resolved Hide resolved
docs/md_book/src/intro.md Outdated Show resolved Hide resolved
docs/md_book/src/reliability/ordering.md Outdated Show resolved Hide resolved
docs/md_book/src/reliability/ordering.md Outdated Show resolved Hide resolved
docs/md_book/src/reliability/reliability.md Outdated Show resolved Hide resolved
@jstnlef
Copy link
Contributor

jstnlef commented Aug 29, 2019

Also clippy is failing

error[E0716]: temporary value dropped while borrowed

   --> src/net/socket.rs:235:52

    |

235 |           let mut processed_packets: Vec<Outgoing> = connection

    |  ____________________________________________________^

236 | |             .gather_dropped_packets()

    | |_____________________________________^ creates a temporary which is freed while still in use

...

249 |               .collect();

    |                         - temporary value is freed at the end of this statement

...

259 |           processed_packets.push(processed_packet);

    |           ----------------- borrow later used here

    |

    = note: consider using a `let` binding to create a longer lived value


error: aborting due to previous error


For more information about this error, try `rustc --explain E0716`.

error: Could not compile `laminar`.


To learn more, run the command again with --verbose.

Terminated

script returned exit code 101

@TimonPost TimonPost changed the title Added some information to the readme Improved documentation Sep 6, 2019
@TimonPost
Copy link
Owner Author

Alright, updated this PR again, any more comments?

Copy link
Contributor

@jstnlef jstnlef left a comment

Choose a reason for hiding this comment

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

Seems fine. We can always update it more in the future. 👍

@TimonPost
Copy link
Owner Author

Awesome, thanks for your review comments

@TimonPost TimonPost merged commit 67dac59 into TimonPost:master Sep 7, 2019
jstnlef pushed a commit to jstnlef/laminar that referenced this pull request Oct 12, 2019
jstnlef pushed a commit that referenced this pull request Oct 12, 2019
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.

None yet

3 participants