Enable "#![warn(missing_docs)]"

[alamb]

Note: migrated from original JIRA: https://issues.apache.org/jira/browse/ARROW-4683

We are moving fast with the Rust implementation and the docs can be ignored at times.  We are starting to get to the point where the project is useful and we should ensure that the docs are up to scratch to avoid hurting adoption.

[alamb]

Comment from Owen Nelson(theomn) @ 2019-03-10T01:53:37.792+0000:

Adding the attribute to arrow, datafusion, and parquet yields around 183 errors currently. I may take a little effort to cover the gap that exists right now.

 As a newcomer to the project, I'm looking for ways to explore the codebase and I'd welcome the chance to take a stab at this, however being so unfamiliar with the internals I may need to refer to the docs on one of the other lang's implementations. I may need to refer to the other implementations anyway just to keep the phrasing consistent. It could take some time.

Any objections to me digging into this?

Additionally, would we prefer to set missing docs to warnings rather than errors now that CI will treat warnings as errors as of ARROW-2409?

Comment from Paddy Horan(paddyhoran) @ 2019-03-11T02:08:31.146+0000:

Hey,

Feel free to dig into this.  I opened the issue as eventually I would like us to have this enabled.  But my plan was always to come back and open smaller documentation related issues that would build up to adding this feature.

One of the first PR's I did was to update the docs of the `datatypes` module so feel free to take it module by module as you learn the code base.

+1 for warnings instead of errors now that you tackled ARROW-2409.

Feel free to reach out if you have questions.

[ByteBaker]

@alamb I'm looking at this to help clear the backlog.

We have #![warn(missing_docs)] in arrow-array, arrow-json, and object-store. Everywhere else it's #[allow(missing_docs)] (notice the absence of !) on specific entities only, and many/most of them do have documentation.

How would you like me to proceed?

[ByteBaker]

We can link #112 with this one as well.

[alamb]

How would you like me to proceed?

It sounds like we should turn on the pragma -- the challenge is likely that you'll have to add lots of documentation to get it to pass I would think

[ByteBaker]

Not a problem. If that's what it takes to clear the issues.

[ByteBaker]

One good thing that's happening is that I'm also finding some of the trivial, but overlooked code-pieces. For example:

    fn write(&mut self, buf: &[u8]) -> Result<usize> {
        let string = match String::from_utf8(buf) { // Should've been `str::from_utf8`, avoids allocation
            Ok(x) => x,
            Err(e) => {
                return Err(Error::new(ErrorKind::InvalidData, e));
            }
        };
        self.data.push_str(&string); // With `str::from_utf8`, a reference won't be necessary
        Ok(string.len())
    }

I'm also changing these as they're not too much work and are trivial changes. I hope that's okay?

[alamb]

I'm also changing these as they're not too much work and are trivial changes. I hope that's okay?

Of course, thank you -- this will be great

[alamb]

If it is possible, it would be great to make the PR easy to review if it:

1. Had no breaking API changes
2. Was easy to quickly reason that it didn't break anything

Maybe breaking into several smaller PRs would also be good

[ByteBaker]

That's what I'm planning too. Every time I'm done with one (or two) workspace member(s), I'll raise a PR and link that here too. And no, I'll make sure to not mix any breaking API changes (if at all, coz that diverts the focus) with this.

[ByteBaker]

I have a question.

There are instances when I see a utility function, called nowhere else but the file it's in, has pub visibility. This also generates a no-docs' warning. Making the function private (coz that's what it appears to be) removes the warning.

Should I do so? Or make it private but add the docs still; or just add the docs? For instance look at this:

/// Create a visual representation of columns
pub fn pretty_format_columns(
    col_name: &str,
    results: &[ArrayRef],
) -> Result<impl Display, ArrowError> {
    let options = FormatOptions::default().with_display_error(true);
    pretty_format_columns_with_options(col_name, results, &options)
}

pub fn pretty_format_columns_with_options(
    col_name: &str,
    results: &[ArrayRef],
    options: &FormatOptions,
) -> Result<impl Display, ArrowError> {
    create_column(col_name, results, options)
}

This is the only call for pretty_format_columns_with_options. Though I have my suspicions against doing so because it's exposed in the API and might be an issue for end-users. What's your thoughts?

[alamb]

Should I do so? Or make it private but add the docs still; or just add the docs? For instance look at this:

In my opinion

1. If there are no explicit tests for the function, make it private
2. If there are tests, make it public with docs

[ByteBaker]

@alamb I see the following enum in parquet::column::writer, can be found in docs. It has no documentation, and it's not referenced to from anywhere in the entire repo. Was added over 4 years ago. I feel like this shouldn't exist. What's your thought?

pub enum Level {
    Page,
    Column,
}

P.S.: Removing this didn't cause any issue at all. Test cases passed as well.

[alamb]

Was added over 4 years ago. I feel like this shouldn't exist. What's your thought?

I think we should mark it deprecated and plan to remove it for the next breaking release