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

Add simple command line arguments and Concise nix output #50

Merged
merged 12 commits into from
Dec 16, 2019

Conversation

snawaz
Copy link
Contributor

@snawaz snawaz commented Dec 15, 2019

Review commit by commit for better experience.

  • Add simple command line arguments
  • Concise nix out
    • Use default values for many parameters to reduce the verbosity of the nix output.
    • Simplify builtins.concatLists [ [a] [b] [c] ] to [a b c] and such.
    • Reduce the output by up to 40% less lines.
  • Bump version
  • Print version as comment to the nix output

@snawaz snawaz self-assigned this Dec 15, 2019
@snawaz
Copy link
Contributor Author

snawaz commented Dec 15, 2019

CI fails though:

error: trait objects without an explicit `dyn` are deprecated
    --> src/buf/buf.rs:1154:30
     |
1154 | fn _assert_trait_object(_b: &Buf) {}
     |                              ^^^ help: use `dyn`: `dyn Buf`
     |
note: lint level defined here
    --> src/lib.rs:71:9
     |
71   | #![deny(warnings, missing_docs, missing_debug_implementations)]
     |         ^^^^^^^^
     = note: #[deny(bare_trait_objects)] implied by #[deny(warnings)]

error: trait objects without an explicit `dyn` are deprecated
    --> src/buf/buf_mut.rs:1167:30
     |
1167 | fn _assert_trait_object(_b: &BufMut) {}
     |                              ^^^^^^ help: use `dyn`: `dyn BufMut`

error: aborting due to 2 previous errors

error: Could not compile `bytes`.

Any idea how to fix this?

Copy link
Member

@trha trha left a comment

Choose a reason for hiding this comment

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

CI fails though:

This change defaulting registry to crates.io breaks the capLints override. Let's just remove it.

src/main.rs Show resolved Hide resolved
overlay/mkcrate.nix Outdated Show resolved Hide resolved
overlay/mkcrate.nix Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@snawaz
Copy link
Contributor Author

snawaz commented Dec 16, 2019

CI fails though:

This change defaulting registry to crates.io breaks the capLints override. Let's just remove it.

I've removed it. Hope it works.

However, I didn't understand why it breaks capLints? Can we use default and still make calLints work? (Given that most projects use crates.io extensively and occasionally other registry).

@snawaz snawaz requested a review from trha December 16, 2019 06:09
@snawaz
Copy link
Contributor Author

snawaz commented Dec 16, 2019

@trha .. addressed the feedbacks: see the latest commit.

Copy link
Member

@trha trha left a comment

Choose a reason for hiding this comment

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

Changes not marked "Required" are optional. Can you also split out the localPatterns fix to another PR and restore the old formatting for features?

src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@trha
Copy link
Member

trha commented Dec 16, 2019

Can we use default and still make capLints work?

We can, but I can't think of any way that doesn't add complexity to the overlay, so let's just leave it there for now.

@trha
Copy link
Member

trha commented Dec 16, 2019

You might need to update mkcrate-nobuild.nix as well. Does nix-shell work on this branch?

@snawaz
Copy link
Contributor Author

snawaz commented Dec 16, 2019

You might need to update mkcrate-nobuild.nix as well. Does nix-shell work on this branch?

No. It didn't. Fixed that too. Thanks.

See the latest commit!

Copy link
Member

@trha trha left a comment

Choose a reason for hiding this comment

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

One last change: Please bring back the old formatting for features, or lines like this would be too long.

@snawaz
Copy link
Contributor Author

snawaz commented Dec 16, 2019

Changes not marked "Required" are optional. Can you also split out the localPatterns fix to another PR and restore the old formatting for features?

Is there any visible cue to Required changes? I dont seem to see any.. 😕

  • I've removed the localPatterns fix from this PR.
  • I think the current formatting for features is good, given that most of the times it has only fewer items in it. Can we fix that if we see that as problem in future?

@snawaz
Copy link
Contributor Author

snawaz commented Dec 16, 2019

One last change: Please bring back the old formatting for features, or lines like this would be too long.

ah. I see. Changed to the old formatting. (We could still make it better, but it is not worth doing now).

@snawaz snawaz requested review from trha and removed request for ebkalderon December 16, 2019 16:16
Copy link
Member

@trha trha left a comment

Choose a reason for hiding this comment

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

Sorry if it seems like I'm nit picking but let's keep it consistent.

src/main.rs Outdated Show resolved Hide resolved
@snawaz snawaz requested a review from trha December 16, 2019 16:26
Copy link
Member

@trha trha left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements! 1170 lines fewer is huge! The next step would be adding a warning to makePackageSet if the overlay version and the code generator version differs.

@snawaz
Copy link
Contributor Author

snawaz commented Dec 16, 2019

Thanks for the improvements! 1170 lines fewer is huge! The next step would be adding a warning to makePackageSet if the overlay version and the code generator version differs.

Yes. Have created the issue for that.

@snawaz snawaz merged commit 5483e01 into master Dec 16, 2019
@snawaz snawaz deleted the better-cmdline branch December 16, 2019 16:35
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.

2 participants