Add matcher to check for charset. #253
Conversation
|
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 #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 git commit --amend
# then change the message
git push --force |
|
@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. |
lib/http.js
Outdated
| Assertion.addMethod('charset', function (value) { | ||
| var headers = this._obj.headers; | ||
| var cs = charset(headers); | ||
| var expected = value.replace("-", ""); |
There was a problem hiding this comment.
Is this to transform 'utf-8' to 'utf8'? If so, I'm thinking that might be too specific a case. What do you think about using @keithamus's suggestion of node-mime instead? (See his comment here #26 (comment)) It seems to be better maintained and documented as well.
There was a problem hiding this comment.
Thanks @austince. I did look the suggestion to use node-mime however as of version 2 charset() has been removed which is why I went with the charset module.
You do need to remove the '-' in order for the comparison to succeed. An improvement might be to convert value to all lowercase first. That satisfies input like:
- "UTF-8"
- "utf-8"
- "UTF8"
- "utf8"
and variants.
There was a problem hiding this comment.
Ah, that's unfortunate about node-mime, but makes sense.
It looks like from the charset code that utf-8 is a special case:
If we're going to use this module, do you think it might be better to handle it as a special case on our end as well, instead of removing the - from all inputs?
I think lowercasing the input value sounds like a good idea!
There was a problem hiding this comment.
Good catch there @austince.
|
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. |
test/http.js
Outdated
| @@ -403,4 +403,13 @@ describe('assertions', function () { | |||
| }).should.throw('expected cookie \'name2\' to have value \'value\' but got \'value2\''); | |||
|
|
|||
| }); | |||
|
|
|||
| it('#charset', function () { | |||
There was a problem hiding this comment.
Can we also add tests where:
- no encoding is specified in the
Content-Type - no
Content-Typeis present
Just to test to the null case, since we've seen there might be some inconsistency in that lib.
There was a problem hiding this comment.
@austince I think I've done what you wanted.
|
The tests are also passing locally, so looking good! |
|
@austince Woah did I forget about this. Got distracted. Sorry. I think I've addressed your concerns now. |
|
@austince Just wondering what's happening with this? |
|
Hi @keithamus @vieiralucas @JamesMessinger, could someone take a look at this and #252 when they get a chance? Thanks! |
|
@keithamus I merged in master, however my job still failed. Looks like an issue with the CI setup to me. |
|
Apologies @kierans, it indeed it. I'll merge this now and we can resolve CI in |
Fixes #26