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

Include param type information when parsing method params #172

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@craigkovatch
Copy link

craigkovatch commented Jan 10, 2019

#145 did not include the type information for method params, and does reflect optional params with a question mark ?.

BEFORE Styleguidist renders only the parameter names, not their types:
public focusCell(rowIdx: number, colIdx?: number): void {
image

AFTER Styleguidist renders the type names correctly:
image

I Don't Know What I'm Doing wrt Stylguidist, really, so if there's anything else I need to update, version number that needs to be bumped, etc. etc.

@craigkovatch

This comment has been minimized.

Copy link

craigkovatch commented Jan 10, 2019

@BenLorantfy would love your review on this if you could spare the time.

@BenLorantfy
Copy link
Contributor

BenLorantfy left a comment

Looks great, just had one comment

Show resolved Hide resolved src/parser.ts Outdated
@@ -393,14 +393,24 @@ export class Parser {
return modifiers;
}

public getParameterInfo(callSignature: ts.Signature) {
public getParameterInfo(callSignature: ts.Signature): Method['params'] {

This comment has been minimized.

@pvasek

pvasek Jan 10, 2019

Collaborator

I think it would make sense to create s specific "types" and use that instead of Method['params'] and in Method definition, something like

export interface MethodParameterType {
        name: string;
        required: boolean;
}
export interface MethodParameter { 
    name: string;
    description?: string | null;
    type: MethodParameterType;
 }

Honestly, I am working with typescript almost every day but had no idea something like that Method['params'] is in this context even possible :)

More importantly it's a good opportunity to think about these types once again. I think the required attribute has nothing to do with the type of the param but with the param itself. Because the param is optional not the type. It would be similar to the PropItem then.

This comment has been minimized.

@craigkovatch

craigkovatch Jan 10, 2019

Done, please check to see if it meets your expectations. Here is the relevant RSG code:

export function ArgumentRenderer({ classes, name, type, description, returns, block, ...props }) {
	const isOptional = type && type.type === 'OptionalType';
	const defaultValue = props.default;
	if (isOptional) {
		type = type.expression;
	}
	return (
		<Group className={block && classes.block} {...props}>
			{returns && 'Returns'}
			{name && (
				<span>
					<Name>{name}</Name>
					{type && ':'}
				</span>
			)}
			{type && (
				<Type>
					{type.name}
					{isOptional && '?'}
					{!!defaultValue && `=${defaultValue}`}
				</Type>
			)}
			{type && description && ` — `}
			{description && <Markdown text={`${description}`} inline />}
		</Group>
	);
}

So it appears it supports nullable types, e.g. colIdx: number?, but not optional arguments, e.g. colIdx?: number. Sigh.

So instead of adding required field to MethodParameterType, I suffixed the param name with ?, which renders correctly in RSG and I think is the correct output for this code, too.

This comment has been minimized.

@pvasek

pvasek Jan 10, 2019

Collaborator

OK, somehow that didn't come to my mind, that there is already styleguidist which expects something else, sorry for that.

If the parameters passed to the ArgumentRenderer are just the Method fields then it seems that there has to be type: 'OptionalType' in the MethodParameterType . But I am not really sure if I read that code properly because there is also returns which I have no idea how that's related to argument. Please, check that component yourself ArgumentRenderer

This comment has been minimized.

@craigkovatch

craigkovatch Jan 10, 2019

Yes, I tried the OptionalType approach, but that displays the type as optional, rather than the parameter, i.e. it would render colIdx: number? rather than colIdx?: number.

I agree that ArgumentRenderer seems to have some strange behavior. Maybe it was a copy/paste of another renderer with some irrelevant cruft?

Show resolved Hide resolved src/parser.ts Outdated

craigkovatch added some commits Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment