search

chaijs / chai-http

HTTP Response assertions for the Chai Assertion Library.
http://chaijs.com/plugins/chai-http
633 stars 113 forks source link

Add matcher to check for charset. #253

Closed kierans closed 5 years ago

kierans commented 5 years ago

Fixes #26

austince commented 5 years ago

Hey @kierans, thanks for the contribution!

We're currently trying to upgrade our release process, which is why your travis builds are failing (but should be fixed with https://github.com/chaijs/chai-http/pull/252).

In the meantime, I'll run this locally when I get a chance. The only things I immediately see that we'll have to do is add this new method to the Typescript definitions (in ./types/index.d.ts) and fix the commit message to something like feat: add charset assertion.

git commit --amend
# then change the message
git push --force
kierans commented 5 years ago

@austince Thanks for the feedback.

As an aside, I like the way you have your docs in the Javascript source, however I haven't been able to find how you pull it all together into your README so I can update the README with the new assertion.

austince commented 5 years ago

Hey @kierans, thanks for updating the PR. We also recently removed the docs-to-readme script, as it didn't work as expected. We should think about a more standard way for building the docs in the future, but for now, you'll have to manually add it if that's alright.

austince commented 5 years ago

The tests are also passing locally, so looking good!

kierans commented 5 years ago

@austince Woah did I forget about this. Got distracted. Sorry.

I think I've addressed your concerns now.

kierans commented 5 years ago

@austince Just wondering what's happening with this?

austince commented 5 years ago

Hi @keithamus @vieiralucas @JamesMessinger, could someone take a look at this and #252 when they get a chance? Thanks!

kierans commented 5 years ago

@keithamus I merged in master, however my job still failed. Looks like an issue with the CI setup to me.

keithamus commented 5 years ago

Apologies @kierans, it indeed it. I'll merge this now and we can resolve CI in master