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

Guidelines, overal crate improvements. #45

Open
TimonPost opened this Issue Oct 18, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@TimonPost
Copy link
Member

TimonPost commented Oct 18, 2018

Here are a couple of things I think who are important for a library like this one. If you see something lac of these issues feel free to open a PR. When I read some code from other libs like this one, but in C++/C I saw that there was a huge shortcoming on most of the following points. Which makes the code difficult to read and understand.

  • Every type, method and some fields should be commented.
  • Calculations should be documented. Whether it would be in a PR or code. But it must be clear what is done.
  • Don't hard code values. We should only use config, consts etc hardcoded values are the root of all evil.
  • Everything should be tested, so feel free to write tests.
  • Documentation.
  • Keep files small better have small files with small pieces of logic than having one file with 1000 lines of logic with multiple types/structs etc. Note that I speak of logic, tests not included.
  • Absolutely no panics/unwraps in the logic we need to have propper error handling. You may have panics/unwraps in test code.
  • Grammar / English improvement. I am not a native English speaker so forgive me when making some mistakes. I am trying to avoid making them in the first place. But some checks could not be wrong.
@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Oct 18, 2018

@TimonPost Good stuff!

I would say we should start using:
#[deny(Warning)] etc attributes
so that we force users to provide doc comments etc.

@Xaeroxe

This comment has been minimized.

Copy link
Contributor

Xaeroxe commented Oct 18, 2018

Over on the main Amethyst repo we implemented a different approach, which was to use warnings most of the time except for on CI, where we added flags to cargo to deny on warnings. This allows you to prototype quickly, but still quality gates the repo. See amethyst/amethyst#1011 and amethyst/amethyst#1025 for more.

@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Oct 18, 2018

@Xaeroxe I like this approach. And we should be following what amethyst/amethyst does anyways since we are another repo under the same org.

bors bot added a commit that referenced this issue Oct 19, 2018

Merge #48
48: Refactor 3: fix easy warnings r=LucioFranco a=CBenoit

Related to #31

I fixed most easy warnings:
- unused import
- unused value
- useless mut
- some unused variables

I also fixed the overflowing_literals warnings, you might want to have a closer look at `sequence_buffer.rs` file diff.

Also, there were missing `#[cfg(test)]` causing unused import warnings when not building tests.

I didn't fix warnings related to Error handling because these may require special attention.

Also there are some unused parameters and unused struct attributes that I believe will be used in the future.

Some remarks:
- Purpose of this attribute is not documented + the attribute itself is never used.
 https://github.com/amethyst/laminar/blob/master/src/net/connection/connection_pool.rs#L23
- These are arbitrarily hardcoded values.
 https://github.com/amethyst/laminar/blob/master/src/net/connection/connection_pool.rs#L29
 https://github.com/amethyst/laminar/blob/master/src/net/connection/connection_pool.rs#L33
 Shall I create constants for them?
- This is unused private method
 https://github.com/amethyst/laminar/blob/master/src/net/socket_state.rs#L174
 Shall we delete it or make it public?

There is a lot of dead code. This is not a good practice (related to #45 )

Co-authored-by: Benoît CORTIER <benoit.cortier@fried-world.eu>
@CBenoit

This comment has been minimized.

Copy link
Contributor

CBenoit commented Oct 19, 2018

Maybe we should add rustfmt verification in CI to make sure formatting is uniform.

@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Oct 19, 2018

@CBenoit yes, this is the plan. Mostly all of the amethyst crates are waiting for rustfmt to 1.0 to formalize that. They are like a month overdue 😄

@LucioFranco LucioFranco referenced this issue Nov 10, 2018

Merged

Add twia14 #118

@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Nov 10, 2018

punting this to 0.2.0

@LucioFranco LucioFranco modified the milestones: 0.1.0, 0.2.0 Nov 10, 2018

@fhaynes

This comment has been minimized.

Copy link
Member

fhaynes commented Nov 10, 2018

I'm going to try to add code coverage for 0.1.0 as part of this.

@fhaynes fhaynes referenced this issue Nov 10, 2018

Merged

45 add kcov #98

bors bot added a commit that referenced this issue Nov 11, 2018

Merge #98
98: 45 add kcov r=TimonPost,LucioFranco a=fhaynes

Experiment in adding kcov reports. relates to #45 

Co-authored-by: Fletcher Haynes <fletcher@capitalprawn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment