-
Notifications
You must be signed in to change notification settings - Fork 668
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
New: Add line/col information for JSON #1297
Changes from 9 commits
800634d
ef67aac
322b7f1
504fdb8
52b760e
aa576f6
266ddde
4d332a0
739180d
3a58198
ac4599c
295309d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1 @@ | ||
{ | ||
"name": 'example', | ||
"version": "0.0.1" | ||
} | ||
invalidJson |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,11 +12,8 @@ | |
import { ucs2 } from 'punycode'; | ||
|
||
import { Category } from 'hint/dist/src/lib/enums/category'; | ||
import { IHint, HintMetadata } from 'hint/dist/src/lib/types'; | ||
import { | ||
Manifest, | ||
ManifestParsed | ||
} from '@hint/parser-manifest/dist/src/types'; | ||
import { IHint, HintMetadata, IJSONLocationFunction } from 'hint/dist/src/lib/types'; | ||
import { ManifestParsed } from '@hint/parser-manifest/dist/src/types'; | ||
import { HintContext } from 'hint/dist/src/lib/hint-context'; | ||
import { HintScope } from 'hint/dist/src/lib/enums/hintscope'; | ||
|
||
|
@@ -46,15 +43,15 @@ export default class ManifestAppNameHint implements IHint { | |
} | ||
}; | ||
|
||
const checkIfPropertyValueIsNotEmpty = async (resource: string, content: string, propertyName: string) => { | ||
const checkIfPropertyValueIsNotEmpty = async (resource: string, content: string, propertyName: string, getLocation: IJSONLocationFunction) => { | ||
if (content && (content.trim() === '')) { | ||
await context.report(resource, null, `Web app manifest should have non-empty '${propertyName}' property value.`); | ||
await context.report(resource, null, `Web app manifest should have non-empty '${propertyName}' property value.`, null, getLocation(propertyName)); | ||
} | ||
}; | ||
|
||
const checkIfPropertyValueIsUnderLimit = async (resource: string, content: string, propertyName: string, shortNameLengthLimit: number) => { | ||
const checkIfPropertyValueIsUnderLimit = async (resource: string, content: string, propertyName: string, shortNameLengthLimit: number, getLocation: IJSONLocationFunction) => { | ||
if (content && (ucs2.decode(content).length > shortNameLengthLimit)) { | ||
await context.report(resource, null, `Web app manifest should have '${propertyName}' property value under ${shortNameLengthLimit} characters.`); | ||
await context.report(resource, null, `Web app manifest should have '${propertyName}' property value under ${shortNameLengthLimit} characters.`, null, getLocation(propertyName)); | ||
|
||
return false; | ||
} | ||
|
@@ -63,7 +60,7 @@ export default class ManifestAppNameHint implements IHint { | |
}; | ||
|
||
const validate = async (manifestParsed: ManifestParsed) => { | ||
const { parsedContent: manifest, resource }: { parsedContent: Manifest, resource: string } = manifestParsed; | ||
const { getLocation, parsedContent: manifest, resource } = manifestParsed; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add the types back? We are trying to go full strict (#576) and I think this is needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really? TypeScript fully infers those types correctly so I'm surprised that they'd be required... I might try a local test case first just to make sure this is necessary in full strict mode - I hate unnecessarily redundant type definitions. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've had some problems in the past believing we were getting one type when it was actually another, or when we changed the return type and nothing complained and then hell happen... |
||
|
||
const name = manifest.name; | ||
|
||
|
@@ -100,8 +97,8 @@ export default class ManifestAppNameHint implements IHint { | |
const shortNameLengthLimit: number = 12; | ||
|
||
await checkIfPropertyExists(resource, name, 'name'); | ||
await checkIfPropertyValueIsNotEmpty(resource, name, 'name'); | ||
await checkIfPropertyValueIsUnderLimit(resource, name, 'name', nameLengthLimit); | ||
await checkIfPropertyValueIsNotEmpty(resource, name, 'name', getLocation); | ||
await checkIfPropertyValueIsUnderLimit(resource, name, 'name', nameLengthLimit, getLocation); | ||
|
||
const shortName: string = manifest.short_name; | ||
const shortNameIsRequired: boolean = name && (name.trim() !== '') && (ucs2.decode(name).length > shortNameLengthLimit); | ||
|
@@ -118,8 +115,8 @@ export default class ManifestAppNameHint implements IHint { | |
} | ||
|
||
await checkIfPropertyExists(resource, shortName, 'short_name'); | ||
await checkIfPropertyValueIsNotEmpty(resource, shortName, 'short_name'); | ||
await checkIfPropertyValueIsUnderLimit(resource, shortName, 'short_name', shortNameLengthLimit); | ||
await checkIfPropertyValueIsNotEmpty(resource, shortName, 'short_name', getLocation); | ||
await checkIfPropertyValueIsUnderLimit(resource, shortName, 'short_name', shortNameLengthLimit, getLocation); | ||
}; | ||
|
||
context.on('parse::manifest::end', validate); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1 @@ | ||
{ | ||
"compilerOptions": { | ||
"alwaysStrict": true, | ||
"declaration": true, | ||
"inlineSourceMap": true, | ||
"lib": [ | ||
'dom', | ||
"dom.iterable", | ||
"es2017", | ||
"esnext", | ||
"esnext.asynciterable" | ||
], | ||
"module": "commonjs", | ||
"newLine": "lf", | ||
"removeComments": false, | ||
"target": "esnext" | ||
}, | ||
"exclude": [ | ||
"dist", | ||
"node_modules", | ||
"packages" | ||
] | ||
} | ||
invalidJson |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import { ProblemLocation } from './problems'; | ||
|
||
export interface IJSONLocationOptions { | ||
at?: 'name' | 'value'; | ||
} | ||
|
||
/** | ||
* Resolve the location of a JSON object path (defaults to property name). | ||
* Pass `true` for `atValue` to get the location of the property value instead. | ||
*/ | ||
export interface IJSONLocationFunction { | ||
(path: string, options?: IJSONLocationOptions): ProblemLocation; | ||
} | ||
|
||
/** | ||
* Access parsed JSON with location information and scoping options. | ||
*/ | ||
export interface IJSONResult { | ||
|
||
/** | ||
* The raw parsed data (as would be returned by `JSON.parse`). | ||
*/ | ||
data: any; | ||
|
||
/** | ||
* Resolve the location of a JSON object path (defaults to property name). | ||
* Pass `true` for `atValue` to get the location of the property value instead. | ||
*/ | ||
getLocation: IJSONLocationFunction; | ||
|
||
/** | ||
* Get a `JSONResult` scoped to the specified path as its root. | ||
* @param path The path to the new root (e.g. `foo.bar`) | ||
*/ | ||
scope(path: string): IJSONResult; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,13 @@ | ||
import * as ajv from 'ajv'; | ||
import { ProblemLocation } from './problems'; | ||
|
||
export interface ISchemaValidationError extends ajv.ErrorObject { | ||
location?: ProblemLocation; | ||
} | ||
|
||
export type SchemaValidationResult = { | ||
data: any; | ||
errors: Array<ajv.ErrorObject>; | ||
errors: Array<ISchemaValidationError>; | ||
prettifiedErrors: Array<string>; | ||
valid: boolean; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alrra and @molant, I changed this because the position was off on my machine, but it looks like the original value was expected in CI. Upon further review, it seems this position is different when running on Windows. I suspect this is due to Windows using
\r\n
instead of\n
for newlines as the difference is exactly the same as the number of lines before the token.I can (1) simply put this back to the old value to pass CI, (2) update the test (and similar tests) to not depend on the exact position, or (3) update how we're loading the test fixture files so we always use
\n
, making the offsets match cross-OS.My preference is (3), but I'd like your opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you are checking out your files with
\r\n
in your machine?What do you think of:
\r\n
git config --global core.autocrlf input
if I understand the guide correctly although maybe there's a way to enforce this in the project's configuration in.gitattributes
?I don't think that should cause any issue with VS Code and will not need any code changes on your side. We might
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this by adding
eol=lf
to.gitattributes
in 3a58198. This ensures files are checked out withLF
even on Windows (the previous setting only ensured that they were committed to the repo with LF, checkout to the working directory was still controlled by user settings).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My recommendation is to not use
eol=lf
as it will bite users in the future. Just use https://nodejs.org/api/os.html#os_os_eol, or something like that to figure out how many characters you need to add.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll remove it.
I'll have to use something slightly different than
os.EOL
to avoid breaking the test case for @molant who already has user settings to checkout with LF even on Windows...I can load the actual JSON test file separately and check for
\r\n
to alter the position if that's fine with you.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about this. Why do you think this can bite users in the future? All Windows editors (including notepad) support this line ending and not having this option has already bite us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.