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

Type classes: Omit explicit instance name #417

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gasi
Copy link

@gasi gasi commented Feb 14, 2022

The compiler change to make instance names optional was introduced in v0.14.2:
https://github.com/purescript/purescript/releases/tag/v0.14.2

The compiler change to make instance names optional was introduced in v0.14.2:
https://github.com/purescript/purescript/releases/tag/v0.14.2
Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

LGTM. However, I'm wondering if we should state that instance names are optional and provide an example.

AFAIK, removing instance names altogether hasn't been discussed and they're still used in core/contrib/web/node libraries. A new user might wonder if they are some advanced syntax or something.

@thomashoneyman
Copy link
Member

thomashoneyman commented Feb 14, 2022

Yea, these instances are preserved across core / contrib until 0.15 because it’s a breaking change to remove them (it means losing support for 0.14.0 and 0.14.1). I think it’d be preferable to mention they’re optional.

I believe there’s another docs page where we talk about instance names being used for readable JavaScript that we could crib from?

@JordanMartinez
Copy link
Contributor

This hasn't been merged because feedback hasn't been addressed. It wouldn't take much to finish it.

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.

3 participants