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

Throw when calling _super on overwriteMethod if method is undefined #528

Merged
merged 1 commit into from
Oct 4, 2015

Conversation

wbyoung
Copy link
Contributor

@wbyoung wbyoung commented Oct 4, 2015

This fixes #467.

@wbyoung wbyoung force-pushed the throw-super-overwrite-method branch from 09858fb to 2f4cd16 Compare October 4, 2015 21:49
keithamus added a commit that referenced this pull request Oct 4, 2015
Throw when calling _super on overwriteMethod if method is undefined
@keithamus keithamus merged commit c772b42 into chaijs:4.x.x Oct 4, 2015
@keithamus
Copy link
Member

👍 thanks @wbyoung

@wbyoung
Copy link
Contributor Author

wbyoung commented Oct 4, 2015

@keithamus sure thing. Sorry for such a long delay between submitting the issue & sending a PR — life's been busy.

@wbyoung wbyoung deleted the throw-super-overwrite-method branch October 4, 2015 22:45
@keithamus
Copy link
Member

Can totally respect that. Thank you for taking the time to do it! I'll let you know when it is released 😄

@electricmonk
Copy link

This is awesome.
I'd very much like to see a change in documentation to encourage plugin authors to respect and share namespaces, like @wbyoung mentions in #467

@keithamus
Copy link
Member

@electricmonk yeah, that is a great point. Care to make a PR for it? All of the plugin guides in Chaijs.com are markdown files here on the chai-docs repo. Please, if you have any insight to put in to the guides (which I'm sure you do), then I encourage you to make a PR 😄

@electricmonk
Copy link

@keithamus Shall I make the PR to the 4.x.x branch?
In general, I would suggest deprecating addMethod and addProperty and only allowing a single interface for registering matchers. But this is a breaking change that would have to be gradually introduced.

@keithamus
Copy link
Member

@electricmonk

Shall I make the PR to the 4.x.x branch?

Yes please!

In general, I would suggest deprecating addMethod and addProperty and only allowing a single interface for registering matchers. But this is a breaking change that would have to be gradually introduced.

Yup, totally. Have a look at #457 which makes small mention of that, you can also see #117 (comment) for some sketching on what a simpler plugin system would look like. I have some more code on my machine but I want to hash out a sensible design before we properly dive into that kind of refactoring. I think I'd like to see us have no "plugin interface", and plugins just export an object of methods which return assertion errors or something similar.

@electricmonk
Copy link

Hmm. @keithamus, the chai-docs repo doesn't seem to have a 4.x.x branch, shall I create it?

@keithamus
Copy link
Member

@electricmonk sorry, for the chai-docs repo if you could please take a look at the gh-pages branch, which is effectively inline with 4.x.x (or, at least, they'll probably be released around the same time).

@electricmonk
Copy link

hey, @keithamus -
Currently the documentation for building a plugin starts from describing addProperty and addMethod, and then describes the overwrite... methods. My thoughts are to deprecate the add... methods in the documentation, so that they are mentioned at the end as backwards-compatibility relics, and focus on overwriteMethod and overwriteProperty.

However, since this means a major overhaul of the helpers page, I thought I'd better ok it with you first. So - are you ok with this? :)

@keithamus
Copy link
Member

@electricmonk I'd rather focus energy on #585. Also I'm not sure overwriteMethod is the one canonical property; it's more complex than addMethod, which will still work for the majority of use cases.

@electricmonk
Copy link

@keithamus so what should be the scope for my docs PR? maybe a note at the top of the page, in the spirit of

Note: the plugin API is pending a major overhaul in future versions of Chai. In the meantime, please prefer overwriteMethod to addMethod for the sake of sharing plugin namespaces.

@keithamus
Copy link
Member

yes that might be a good idea, thanks.

@electricmonk
Copy link

@keithamus It appears that the helpers page in the gh-pages branch doesn't have the addMethod section (although it appears in the ToC). Is that intentional?

@keithamus
Copy link
Member

That certainly isn't intentional, the gh-pages site is a work in progress, so its likely something that got missed. If you could so kindly add it back in that'd be swell 😄

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