Skip to content

Conversation

@ykhandor
Copy link
Contributor

Enable fileNameGenerator "byType" to read Javscript files by mimeType. Currently code only enables CSS and HTML files by mimeType.

…ently code only enables CSS and HTML files by mimeType.
Copy link
Member

@s0ph1e s0ph1e left a comment

Choose a reason for hiding this comment

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

Thanks for contribution 👍

Could you please add small test for js extension, like we have for css and html here

it('should add missed extensions for html resources', function () {
var r = new Resource('http://example.com/about', '');
r.getType = sinon.stub().returns('html');
var filename = byTypeFilenameGenerator(r, {}, []);
filename.should.equalFileSystemPath('about.html');
});
it('should add missed extensions for css resources', function () {
var r = new Resource('http://example.com/css', '');
r.getType = sinon.stub().returns('css');
var filename = byTypeFilenameGenerator(r, {}, []);
filename.should.equalFileSystemPath('css.css');
});
?

'.css': types.css
'.css': types.css,
'.js': types.js,
'.min.js': types.js
Copy link
Member

Choose a reason for hiding this comment

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

I believe .min.js is not needed here, such files will have extension.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense..will get rid of this one. I was not sure, I added it to be double sure and cover all bases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I will add the test for .js files and push the code up. You should have a new commit/PR to approve soon!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s0ph1e : The updated code has been pushed and the new commit is in and ready for approval.

@s0ph1e
Copy link
Member

s0ph1e commented Jul 24, 2018

Thanks @ykhandor
I will publish this changes in next release 3.3.4 today

@s0ph1e s0ph1e merged commit 5ed0f62 into website-scraper:master Jul 24, 2018
@s0ph1e
Copy link
Member

s0ph1e commented Jul 24, 2018

Released in version 3.3.5

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.

2 participants