gh-bitflags/bitflags

Watch

Fork 0

"flags" is meaningless #67

New issue

Closed

opened 2017-03-06 23:12:59 -06:00 by dtolnay · 5 comments

dtolnay commented

2017-03-06 23:12:59 -06:00

(Migrated from github.com)

This threw me off when reading the bitflags documentation for the first time.

bitflags! {
    flags Flags: u32 {
        /* ... */
    }
}

I know it is not a keyword because my IDE does not highlight it like one. The casing suggests that it is a variable name or module name. Nope, it is a nugatory token required by the macro.

I think the important relevant thing to communicate to readers is that a type with the given name is being declared by the macro. Either of the following would be far more valuable I think.

bitflags! {
    type Flags: u32 {
        /* ... */
    }
}

bitflags! {
    struct Flags: u32 {
        /* ... */
    }
}

This threw me off when reading the bitflags documentation for the first time. ```rust bitflags! { flags Flags: u32 { /* ... */ } } ``` I know it is not a keyword because my IDE does not highlight it like one. The casing suggests that it is a variable name or module name. Nope, it is a nugatory token required by the macro. I think the important relevant thing to communicate to readers is that a type with the given name is being declared by the macro. Either of the following would be far more valuable I think. ```rust bitflags! { type Flags: u32 { /* ... */ } } ``` ```rust bitflags! { struct Flags: u32 { /* ... */ } } ```

brson commented

2017-03-09 20:34:38 -06:00

(Migrated from github.com)

Let's change this to "struct". We don't need to preserve backwards compatibility.

vedantrathore commented

2017-03-13 18:07:05 -05:00

(Migrated from github.com)

@brson so I have to change flags to struct here and here right?

@brson so I have to change `flags` to `struct` [here](https://github.com/rust-lang-nursery/bitflags/blob/master/src/lib.rs#L114) and [here](https://github.com/rust-lang-nursery/bitflags/blob/master/src/lib.rs#L68) right?

brson commented

2017-03-20 17:38:47 -05:00

(Migrated from github.com)

@vedantrathore yes, but those locations are only in documentation. The most important place to change is in the macro defenition itself, like here and here and here.

Once you've changed the macro definition, run cargo test. That will test the macro and the documentation examples. If the tests pass you are likely done and can submit a PR. Sorry for taking so long to get back to you. Feel free to ping me on IRC on send an email to get ahold of me faster.

@vedantrathore yes, but those locations are only in documentation. The most important place to change is in the macro defenition itself, like [here](https://github.com/rust-lang-nursery/bitflags/blob/master/src/lib.rs#L179) and [here](https://github.com/rust-lang-nursery/bitflags/blob/master/src/lib.rs#L196) and [here](https://github.com/rust-lang-nursery/bitflags/blob/master/src/lib.rs#L208). Once you've changed the macro definition, run `cargo test`. That will test the macro and the documentation examples. If the tests pass you are likely done and can submit a PR. Sorry for taking so long to get back to you. Feel free to ping me on IRC on send an email to get ahold of me faster.

vedantrathore commented

2017-03-20 17:56:18 -05:00

(Migrated from github.com)

@brson no problem :) thanks for the info, I'll get onto it.

crumblingstatue commented

2017-05-07 04:03:58 -05:00

(Migrated from github.com)

I believe this issue can be closed now, see #84.

No labels

bug

good first issue

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-bitflags/bitflags#67

No description provided.

Rows
Columns