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

ScalaDocs improvement for utils Math, MixedVec #1019

Merged
merged 9 commits into from
Mar 11, 2019
Merged

Conversation

ducky64
Copy link
Contributor

@ducky64 ducky64 commented Feb 16, 2019

Adds recommended usage and examples to the Math utils (log2* and isPow2).
Slight improvement to documentation for MixedVec, primarily deduping method ScalaDoc into the object / class ScalaDoc, and clarifying wires / types.

Related issue: #411

Type of change: other enhancement

Impact: no functional change

Development Phase: implementation

Release Notes
Scaladocs for Math (log2*, isPow2) and MixedVec.

@ducky64 ducky64 requested a review from a team as a code owner February 16, 2019 02:59
@ducky64 ducky64 changed the title Duckydocs ScalaDocs improvement for utils Math, MixedVec Feb 16, 2019
@ducky64
Copy link
Contributor Author

ducky64 commented Feb 16, 2019

I was also supposed to write docs for Lookup, but I think we should have a discussion on whether to keep those in util, since I'm not sure if they're used, and they seem like very special-purpose constructs that might be better done by composing other functionality like PriorityMux and Vec.

Copy link
Contributor

@edwardcwang edwardcwang left a comment

Choose a reason for hiding this comment

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

In general is there a reason to put docs into the object as opposed to the actual apply method?

* This will be deprecated when zero-width wires is supported.
*
* @example {{{
* log2Up(1) = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the example be executable? Maybe something like assert(log2Up(1) == 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have stylistic conventions on that? That's probably good to figure out.

Copy link
Contributor

Choose a reason for hiding this comment

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

poke?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Didn't realize this was a blocking issue. I opted to put the return value in comments because the assert is not part of the example.

* zero-width wires.
*
* @example {{{
* log2Up(1) = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this object log2Ceil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed... derp

@ducky64
Copy link
Contributor Author

ducky64 commented Feb 16, 2019

Didn't we discuss putting docs in objects vs. methods during the beginning of the meeting (if that was changed later, then I probably missed it)? I think the reasoning was so that you don't have to expand the function defs when you navigate to the object page.

@seldridge
Copy link
Member

Yeah, put it into the objective and not the method if you have big examples. Take a look at how it comes out in the scaladoc.

@ducky64
Copy link
Contributor Author

ducky64 commented Feb 26, 2019

Is there anything blocking review approval for this PR?

@ducky64
Copy link
Contributor Author

ducky64 commented Mar 11, 2019

Poke?

Copy link
Contributor

@edwardcwang edwardcwang left a comment

Choose a reason for hiding this comment

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

LGTM

@edwardcwang edwardcwang merged commit 275515d into master Mar 11, 2019
@edwardcwang edwardcwang deleted the duckydocs branch March 11, 2019 12:09
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