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.

Sign up for GitHub

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

Jump to bottom

cargo-marker: add initial configuration support #84

Merged
merged 7 commits into from
Jan 10, 2023
Merged

cargo-marker: add initial configuration support #84

merged 7 commits into from
Jan 10, 2023

Conversation

Niki4tap
Copy link
Member

@Niki4tap Niki4tap commented Jan 9, 2023

Tracked in #81

Adds initial configuration implementation, checking out one task :)

@xFrednet

@Niki4tap Niki4tap mentioned this pull request Jan 9, 2023
Closed
@xFrednet xFrednet self-assigned this Jan 9, 2023
xFrednet
xFrednet reviewed Jan 9, 2023
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, some nits and comments on design decisions. I'm fine with leaving some of them open. But then they should be tracked in an issue and have a comment in code.

A nice start, thank you for the support!!!

Cargo.toml Outdated Show resolved Hide resolved
cargo-marker/src/config.rs Outdated Show resolved Hide resolved
cargo-marker/src/config.rs Show resolved Hide resolved
cargo-marker/src/main.rs Outdated Show resolved Hide resolved
@xFrednet xFrednet added A-marker-cargo Area: All things connected to `cargo_marker` S-waiting-on-review Status: Awaiting review labels Jan 9, 2023
@Niki4tap
Copy link
Member Author

Thanks again for the review!

I addressed the comments, did some refactoring, and I'm relatively happy with the code now, can you review it again please?

xFrednet
xFrednet reviewed Jan 10, 2023
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Generally, looks good to me, a few more NITs (Sorry if I'm too picky) and then this should be ready to go :)

cargo-marker/src/config.rs Show resolved Hide resolved
cargo-marker/src/lints.rs Show resolved Hide resolved
cargo-marker/src/lints.rs Outdated Show resolved Hide resolved
cargo-marker/src/lints.rs Outdated Show resolved Hide resolved
xFrednet
xFrednet approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!!

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 10, 2023

Build succeeded:

@bors bors bot merged commit 2c1582d into rust-marker:master Jan 10, 2023
@Niki4tap Niki4tap deleted the marker_config branch January 10, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

Labels
A-marker-cargo Area: All things connected to `cargo_marker` S-waiting-on-review Status: Awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Niki4tap @xFrednet