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.

Sign up for GitHub

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

Jump to bottom

Check for mozIndexedDB; close #15 #44

Merged
merged 2 commits into from
Oct 6, 2015
Merged

Check for mozIndexedDB; close #15 #44

merged 2 commits into from
Oct 6, 2015

Conversation

tofumatt
Copy link
Contributor

@tofumatt tofumatt commented Oct 4, 2015

This adds support for custom ESLint rules and tests for the existence of mozIndexedDB.

It adds a getFileAsString() method to the Xpi class so we can load entire files into memory to pass to linters like ESLint, which needs the entire file and can't deal with streams. As discussed earlier on IRC, we might not end up passing streams around much when maybe checks need the full context of a file.

@tofumatt tofumatt assigned tofumatt and unassigned muffinresearch Oct 5, 2015
@tofumatt tofumatt force-pushed the add-mozindexeddb-test branch from 4496127 to dbfd899 Compare October 5, 2015 09:43
@tofumatt tofumatt assigned muffinresearch and unassigned tofumatt Oct 5, 2015
@tofumatt
Copy link
Contributor Author

tofumatt commented Oct 5, 2015

I think this is ready for r? now.

muffinresearch
muffinresearch reviewed Oct 5, 2015
View reviewed changes
tests/test.javascript.js
var code = 'var myDatabase = indexeddb || mozIndexedDB;';
var jsScanner = new JavaScriptScanner(code, 'badcode.js');

jsScanner.scan()
Copy link
Contributor

Choose a reason for hiding this comment

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

If you return a promise in a test you don't need to use done() it's actually quite useful as this will expose issues where the code misses returning a promise too.

Copy link
Contributor Author

@tofumatt tofumatt Oct 5, 2015 via email

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's handy, I only found out about this recently too.

@muffinresearch
Copy link
Contributor

Looks pretty good overall - just a few things that need addressing.

Would be good to fix up the returning promises in the tests that feature promise-based apis rather than using done() unless there's specific reasons why that doesn't work.

It seems it's important to understand any limitations of eslint can a test be easily bypassed through obfuscation via concatenated propnames. Maybe FreddyB has seen good ways to handle that? Also that require / import question you raised is of a similar nature.

There's a couple of missing tests, e.g. just a basic one for the util and also I think we need some to make sure errors are being handled should eslint blow-up or the js-validator blow-up in the iteration over the js files in the xpi tests.

The rest are minor nits.

@muffinresearch
Copy link
Contributor

Give me shout if you want to chat over any of these bits. Some of it we might want to move to future bugs - and that's fine to do. We just need to be careful we don't end up with a huge list of things that mean that eslint for validation is easily bypassed.

muffinresearch
muffinresearch reviewed Oct 5, 2015
View reviewed changes
src/const.js
@@ -1,6 +1,10 @@
export const DEFLATE_COMPRESSION = 8;
export const NO_COMPRESSION = 0;

export const SEVERITY_ERROR = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this not use VALIDATION_* instead. Seems it might get confusing to have uses of similar but different constants. I'm easily confused :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's better. I used severity as it's what ESLint uses internally, but I didn't like it either. VALIDATION_ it is! 😄

@tofumatt tofumatt mentioned this pull request Oct 6, 2015
Closed
@muffinresearch muffinresearch added 44.2 and removed 44.1 labels Oct 6, 2015
@tofumatt tofumatt mentioned this pull request Oct 6, 2015
Closed
@tofumatt tofumatt force-pushed the add-mozindexeddb-test branch from 271afd5 to d75b539 Compare October 6, 2015 16:51
@tofumatt tofumatt assigned muffinresearch and unassigned tofumatt Oct 6, 2015
@tofumatt
Copy link
Contributor Author

tofumatt commented Oct 6, 2015

Hey there, updated the PR. Should have fixed everything. Let me know if that test in validator is still too much and we can take it out, but it should be less brittle now.

I liked testing against an actual file, but if it's not discrete enough let me know.

muffinresearch
muffinresearch reviewed Oct 6, 2015
View reviewed changes
src/validator.js
addToCollector(message) {
var messageType = message.severity.charAt(0).toUpperCase() +
message.severity.slice(1);
console.log(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably remove this - though at some point it would be nice to have some configurable logging - we should add an issue to add something with debug levels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, yes, this is totally unintentional.

@muffinresearch
Copy link
Contributor

Nice one! Couple of minor nits left but looks good r+wc.

@tofumatt
Copy link
Contributor Author

tofumatt commented Oct 6, 2015

Thanks! All the nits are fixed now; gonna squash a few commits then merge it.

@tofumatt tofumatt force-pushed the add-mozindexeddb-test branch from ec0eb3e to 103ad02 Compare October 6, 2015 22:18
@tofumatt
Check for mozIndexedDB; close #15
a43995b 
This adds support for custom ESLint rules. It will probably conflict
with #43. It's been modified to make use of the messages structure in there.

We can merge #43 in first, then do this.
@tofumatt
Remove unused SIGNING_SEVERITIES
e8014c2
@tofumatt tofumatt force-pushed the add-mozindexeddb-test branch from 5a475cb to e8014c2 Compare October 6, 2015 22:23
tofumatt added a commit that referenced this pull request Oct 6, 2015
@tofumatt
Merge pull request #44 from mozilla/add-mozindexeddb-test
af07eaf 
Check for mozIndexedDB; close #15
@tofumatt tofumatt merged commit af07eaf into master Oct 6, 2015
@tofumatt tofumatt deleted the add-mozindexeddb-test branch October 6, 2015 22:29
@tofumatt tofumatt removed the 44.2 label Oct 7, 2015
@data-sync-user data-sync-user mentioned this pull request Oct 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@muffinresearch muffinresearch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tofumatt @muffinresearch