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

Use macros for module declarations on fmt module #1075

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

k-nasa
Copy link
Contributor

@k-nasa k-nasa commented Oct 31, 2020

Motivation

ref: #1009

This is code refactoring. Currently, there is a lot of code that declares modules as follows:

#[cfg(feature = "registry")]
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
pub use registry::Registry;

#[cfg(feature = "registry")]
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
pub fn registry() -> Registry {
    Registry::default()
}

It is verbose to write a module declaration multiple times using the feature attribute. Also, if the definition for the same feature spans multiple places, it will be a little harder to read the code.

Solution

You can combine features attributes in one place by writing as follows. It also eliminates the need to write the same code over and over again.

cfg_feature!("feature" ,{
    pub use registry::Registry;

    pub fn registry() -> Registry {
        Registry::default()
    }
});

@k-nasa k-nasa requested review from hawkw and a team as code owners October 31, 2020 08:17
@davidbarsky
Copy link
Member

Thanks! Can you format this PR to pass CI?

@k-nasa
Copy link
Contributor Author

k-nasa commented Nov 3, 2020

Do you know why ci displays an error?

@hawkw
Copy link
Member

hawkw commented Nov 21, 2020

Looks like some merge conflicts need to be resolved?

@bryangarza
Copy link
Member

We could probably still re-base and get this merged fyi @k-nasa

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.

4 participants