Skip to content

Conversation

@MartynasZilinskas
Copy link
Collaborator

No description provided.

Copy link
Member

@DeividasBakanas DeividasBakanas left a comment

Choose a reason for hiding this comment

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

Non functional changes requested.

const referenceTuples = GetApiItemReferences(extractedData, [[apiItem.Name, apiItem.ApiItems]]);
list = list.concat(referenceTuples);

const referencesList = GetApiItemReferences(extractedData, [{ Alias: apiItem.Name, Ids: apiItem.ApiItems }]);
Copy link
Member

Choose a reason for hiding this comment

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

Move to separate const just like in line 144.

itemsReference: Contracts.ApiItemReferenceTuple
itemsReference: Contracts.ApiItemReference[]
): ApiItemReference[] {
let list: ApiItemReference[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Consider changing variable list name to overallReferences.

return apiItems;
}

export function LogWithApiItemPosition(logLevel: LogLevel, apiItem: Contracts.ApiItemDto, message: string): void {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe default value for logLevel would be handy?

export function LogWithApiItemPosition(apiItem: Contracts.ApiItemDto, message: string, logLevel: LogLevel = LogLevel.Warning): void

(default value can be other than LogLevel.Warning)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not really sure if it's right decision. Because default Log function for logging is:

Log: (level: LogLevel, ...messages: any[]) => number;

@MartynasZilinskas MartynasZilinskas merged commit 16fd42e into dev Dec 22, 2017
@MartynasZilinskas MartynasZilinskas deleted the feature/upgrade branch December 22, 2017 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants