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

refactor: Flatten autoendpoint server module #182

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

AzureMarker
Copy link
Contributor

@AzureMarker AzureMarker commented Jul 10, 2020

This PR will likely require some merge conflict resolution and rebasing, but I wanted to get it out so we fix this structure issue sooner rather than later.

Closes #157

@AzureMarker AzureMarker added this to the Autoendpoint Rust Server milestone Jul 10, 2020
@AzureMarker AzureMarker linked an issue Jul 10, 2020 that may be closed by this pull request
@AzureMarker AzureMarker force-pushed the feat/autoendpoint-fcm-router branch from 64c6f5f to 14abb92 Compare July 13, 2020 18:19
@AzureMarker AzureMarker force-pushed the refactor/flatten-autoendpoint branch from ce9a7ce to b444dc2 Compare July 13, 2020 18:34
@AzureMarker AzureMarker marked this pull request as ready for review July 17, 2020 19:07
jrconlin
jrconlin previously approved these changes Jul 17, 2020
use crate::server::extractors::notification_headers::NotificationHeaders;
use crate::server::extractors::subscription::Subscription;
use crate::extractors::notification_headers::NotificationHeaders;
use crate::extractors::subscription::Subscription;
Copy link
Member

Choose a reason for hiding this comment

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

optional, could probably collapse these:

use crate::extractors::{
    notification_headers:::NotificationHeaders,
    subscription::Subscription,
};

if you do, just need to be consistent with other, similar includes. Could save a bit of typing for any future moves like this.

Copy link
Contributor Author

@AzureMarker AzureMarker Jul 20, 2020

Choose a reason for hiding this comment

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

We can make rustfmt handle this automatically:
https://rust-lang.github.io/rustfmt/?version=master&search=#merge_imports

Unfortunately this currently requires nightly, until rustfmt 2.0 is released. The reason is that it is an unstable style feature because it has the potential to break code.

Alternatively, do we really want this? I agree it's more consistent and saves typing, but I've also seen the argument that it is more prone to merge conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's the clippy battle. I think clippy really wanted folks to collapse when possible, probably because of tons of one line uses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, is Clippy complaining about this? I don't see any relevant lints in https://rust-lang.github.io/rust-clippy/stable/index.html

autoendpoint/src/error.rs Show resolved Hide resolved
Base automatically changed from feat/autoendpoint-fcm-router to master July 20, 2020 13:16
@AzureMarker AzureMarker dismissed jrconlin’s stale review July 20, 2020 13:16

The base branch was changed.

@AzureMarker AzureMarker force-pushed the refactor/flatten-autoendpoint branch from b444dc2 to 4b294f9 Compare July 20, 2020 13:50
@AzureMarker AzureMarker requested a review from jrconlin July 20, 2020 13:59
@AzureMarker AzureMarker merged commit a12e209 into master Jul 21, 2020
@AzureMarker AzureMarker deleted the refactor/flatten-autoendpoint branch July 21, 2020 13:19
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.

[autoendpoint] Flatten module structure a bit
2 participants