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

2018 edition #222

Merged
merged 18 commits into from
Sep 16, 2019
Merged

2018 edition #222

merged 18 commits into from
Sep 16, 2019

Conversation

zrzka
Copy link
Contributor

@zrzka zrzka commented Sep 16, 2019

2018 edition changes

  • All crates (Cargo.toml) do contain edition = "2018"
  • All occurrences of extern crate ... removed
  • Macros imported like any other types

Warnings

  • Replaced ONCE_INIT with Once::new
  • Prefixed all subcrate examples with subcrate abbreviation
    • Example crossterm_cursor/examples/cursor.rs -> crossterm_cursor/examples/cc_cursor.rs
    • I agree that it's ugly and that it has to be solved somehow, but it's polluting build logs and will be a hard error in the future, take it as a temporary fix

Imports

All imports are semantically ordered. The order is:

  • Standard library
  • External crates
  • crate
  • super
  • self
  • mod

I do not expect that everyone is going to maintain this order, but I consider it as a good practice to make the code more readable.

Fixes #213

@zrzka zrzka mentioned this pull request Sep 16, 2019
@TimonPost
Copy link
Member

Awesome! Yea I had an other order I maintained import order for a while. Thanks for putting in the work for the arranging them to an other order. I will try to keep that as default, I might want to create some code_style.md.

I'm really doubting that we might want to remove the examples from the sub crates and instead use those in /examples, because all those examples are mostly duplicated with the only difference being the namespaces

@zrzka
Copy link
Contributor Author

zrzka commented Sep 16, 2019

I'm really doubting that we might want to remove the examples from the sub crates and instead use those in /examples, because all those examples are mostly duplicated with the only difference being the namespaces.

Yeah, that's the reason why I just prefixed them with an abbreviation (to fix warnings) and made a separate issue for this. I'm not aiming to solve the examples in this PR.

@zrzka
Copy link
Contributor Author

zrzka commented Sep 16, 2019

Don't merge yet, I've just found that I missed the examples/program_examples. Going to fix them right now.

@zrzka
Copy link
Contributor Author

zrzka commented Sep 16, 2019

Okay, I think it's ready for review. Couple of new things:

  • Missed some extern crate ...
  • Converted examples/ as well
  • Removed extern crate from docs
  • Added build & formatting check (stable only) for examples/program_examples

Copy link
Member

@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.

I like that some of the mod.rs files are removed, and moved a level up. Also, I like that all the crate imports are removed. There is not much to say about the changes, so thanks!

#[cfg(windows)]
use crossterm_utils::supports_ansi;
use crossterm_utils::{impl_display, Command, Result};

use crate::{Color, ITerminalColor};

use super::*;
Copy link
Member

Choose a reason for hiding this comment

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

Eventually, I like to get rid of those as well, doesn't need to be done in this PR tough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate more? Why to remove crate::...? Replace super::* with specific imports to avoid *?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, * is unclear to which types your importing and can create confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll do this. But if you don't mind, in a separate PR, cause this one is mainly about 2018 edition.

Copy link
Member

Choose a reason for hiding this comment

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

Yea totally fine

crossterm_style/src/lib.rs Outdated Show resolved Hide resolved
@TimonPost
Copy link
Member

Just give the sign and I merge it

@zrzka
Copy link
Contributor Author

zrzka commented Sep 16, 2019

There's no more things to do in this PR. I mean, 2018 edition related. I think it can be merged and comments can be addressed in another PRs.

@TimonPost TimonPost merged commit 5494525 into crossterm-rs:master Sep 16, 2019
@zrzka zrzka deleted the zrzka/2018-edition branch September 16, 2019 11:35
december1981 pushed a commit to december1981/crossterm that referenced this pull request Oct 26, 2023
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.

2018 edition
2 participants