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

[interfaces] Expose configuration options for external libraries #51

Merged
merged 5 commits into from
May 25, 2021

Conversation

julien-c
Copy link
Member

@julien-c julien-c commented May 21, 2021

Reviewers: see also upcoming PR in moon-landing [internal]

Copy link
Member

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

Awesome! This is very useful :)

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Great, this is clean, only nits.

snippet: (model: ModelData) => string;
}

//#region snippets
Copy link
Member

Choose a reason for hiding this comment

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

Are those comments for vscode?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes sir

Copy link
Member Author

Choose a reason for hiding this comment

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

though they might be some sort of cross-editor standard syntax? not sure

/**
* Add your new library here.
*/
export enum ModelLibrary {
Copy link
Member

Choose a reason for hiding this comment

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

Could the ModelLibrary be built from the MODEL_LIBRARIES_UI_ELEMENTS so that users contributors have less to fill? It seems to be close to 1-to-1 with the btnLabel in the MODEL_LIBRARIES_UI_ELEMENTS

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could remove the btnLabel if indeed we expect it to already be the same string.

interfaces/README.md Outdated Show resolved Hide resolved
Co-authored-by: Lysandre Debut <lysandre@huggingface.co>
@julien-c julien-c merged commit 5261a4d into main May 25, 2021
@julien-c julien-c deleted the expose_libraries_setup branch May 25, 2021 14:54
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.

None yet

4 participants