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 specific never search #49757

Merged
merged 10 commits into from
Apr 22, 2018
Merged

Add specific never search #49757

merged 10 commits into from
Apr 22, 2018

Conversation

GuillaumeGomez
Copy link
Member

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2018
@shepmaster
Copy link
Member

r? @QuietMisdreavus

@novacrazy
Copy link

Would it not be better to add some kind of generic syntax alias system?

For example, while this may fix searching for !, many other things like ? and any operators (+, %, /, *, [/], etc.) still give weird results.

@GuillaumeGomez
Copy link
Member Author

This is an excellent point! I'll update this PR with these new adds.

@novacrazy
Copy link

novacrazy commented Apr 8, 2018

Maybe something like this?

query = (function() {
    switch(raw) {
        case '!': return "Never";
        case '?': return "try!";
        case '+': return "Add";
        case '-': return "Sub";
        case '%': return "Rem";
        case '[':
        case ']': return "Index";
        // and so forth
        default: return raw;
    }
})();

@kennytm
Copy link
Member

kennytm commented Apr 8, 2018

[] can also refer to slices, and - could also mean the Neg trait. Could query support unioning results? 🤔

@GuillaumeGomez
Copy link
Member Author

I think I'll need to introduce a new syntax. Something like 'vec|Neg' and then it'll look for the both. What do you think of it?

@novacrazy
Copy link

Perhaps a comma would work better? As if it’s a list of search terms rather than a full expression. That would leave | free for aliasing to BitOr as well.

@kennytm
Copy link
Member

kennytm commented Apr 8, 2018

💡 Alternative: Allow items to register symbols as an alias index, like how the primitives are presented.

Rationale: Non-std docs don't care about ! being the never type. Modifying the *.js infects those non-std docs.

#[doc(primitive = "never")]
#[doc(alias = "!")]
/// blah
mod prim_never { }

#[doc(primitive = "slice")]
#[doc(alias = "[]")]
mod prim_slice { }

#[doc(alias = "-")]
pub trait Neg { }

#[doc(alias = "-")]
pub trait Sub { }

@novacrazy
Copy link

novacrazy commented Apr 8, 2018

Is there a way to make a doc attribute like that require an unstable feature flag? That’s not really my area.

I feel like it could be abused in crates, yet it should be allowed in crates on the rare occasion users need to define their own lang items, like with no_core bare-metal projects.

Edit: Can no_core crates even build documentation?

@kennytm
Copy link
Member

kennytm commented Apr 8, 2018

@novacrazy

Is there a way to make a doc attribute like that require an unstable feature flag?

Yes, that's how #[doc(cfg/masked/spotlight)]'s feature-gating works.

@GuillaumeGomez
Copy link
Member Author

@kennytm: This is a brilliant idea. I'm just a bit afraid that people might misuse it though or that readers won't be aware of it, and therefore not use it... But this remains an excellent idea. I think I'll start implementing it.

@shepmaster
Copy link
Member

that people might misuse it though

You could put restrictions that the "alias" has to be non-word characters to start with. This might mitigate overall abuse.

@GuillaumeGomez
Copy link
Member Author

Indeed!

@QuietMisdreavus
Copy link
Member

+1 for adding a general mechanism. Adding a feature gate to a doc-attribute parameter is easy. The compiler already whitelists the doc attribute as a whole, so adding a check for a specific parameter just involves knowing the right place to put it.

Edit: Can no_core crates even build documentation?

@novacrazy Yes - as long as the compiler can build it, so can rustdoc. For example, libcore is a #![no_core] crate, and its docs build fine.

@pietroalbini pietroalbini added T-dev-tools-rustdoc S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2018
@pietroalbini
Copy link
Member

Blocked on #49966

@kennytm
Copy link
Member

kennytm commented Apr 16, 2018

@pietroalbini My alternative in #49757 (comment) wouldn't need #49966, though if the alternative is chosen, this PR should be closed or entirely rewritten.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 17, 2018
@bors
Copy link
Contributor

bors commented Apr 17, 2018

☔ The latest upstream changes (presumably #50033) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

@kennytm: Your doc alias can be the same for multiple types so it still strongly requires the multi-query feature.

@kennytm
Copy link
Member

kennytm commented Apr 18, 2018

@GuillaumeGomez I don't think so, I expect there would be multiple dedicated entries named [] or - etc, and the search UI would be like:

screenshot_2018-04-18 23 57 50_jlnaq0

@GuillaumeGomez
Copy link
Member Author

Ok, here's all the implemented functionality. I finally went for last @kennytm's comment: no need for multiquery, I just add types at the beginning of the results and we're good!

cc @QuietMisdreavus

@rust-highfive

This comment has been minimized.

// `Never` primitive type.
if (raw === '!') {
query = 'Never';
}
Copy link
Member

Choose a reason for hiding this comment

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

We could delete this code now right? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh damn! Completely forgot about it 😆

@GuillaumeGomez
Copy link
Member Author

Updated.

@QuietMisdreavus
Copy link
Member

Looks good! One round of requests from me:

  • Can you open up a tracking issue for the new doc_alias feature and reference it in your feature gate?
  • Could you write up some docs in src/doc/unstable-book/src/language-features/doc-alias.md talking about what the attribute does?
    • Optional: add a note in src/doc/rustdoc/src/unstable-features.md alongside the other doc attribute stuff linking to the Unstable Book page

@QuietMisdreavus
Copy link
Member

Great, thanks so much! r=me pending travis.

@rust-highfive

This comment has been minimized.

@kennytm
Copy link
Member

kennytm commented Apr 22, 2018

@bors r=QuietMisdreavus

@bors
Copy link
Contributor

bors commented Apr 22, 2018

📌 Commit 1ed3e77 has been approved by QuietMisdreavus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 22, 2018
@bors
Copy link
Contributor

bors commented Apr 22, 2018

⌛ Testing commit 1ed3e77 with merge bbdd1cf...

bors added a commit that referenced this pull request Apr 22, 2018
@bors
Copy link
Contributor

bors commented Apr 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing bbdd1cf to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants