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

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@antross
Member

antross commented Sep 7, 2018

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Fixes #1282. Also updates these packages with line/col data:

  • hint-babel-config
  • hint-manifest-app-name
  • hint-manifest-is-valid
  • hint-typescript-config
  • parser-babel-config
  • parser-manifest
  • parser-typescript-config

antross added some commits Sep 5, 2018

New: Add line/col information for JSON
Currently focused on web app manifest hints.
Chore: Refactor access to JSON location
Reduces the knowledge needed by hints.
Chore: Add unit tests for JSON locations
Includes fixing a few minor bugs this kicked up.

@antross antross requested review from alrra, molant and sarvaje as code owners Sep 7, 2018

@antross antross referenced this pull request Sep 7, 2018

Closed

New: Add extension for VSCode #1254

14 of 15 tasks complete
@antross

This comment has been minimized.

Member

antross commented Sep 7, 2018

@alrra, @molant Do we want to update the remaining existing JSON-based parsers and hints as part of this PR? I think only parser-babel-config and parser-typescript-config along with their associated hints are left.

@molant

This comment has been minimized.

Member

molant commented Sep 7, 2018

Do we want to update the remaining existing JSON-based parsers and hints as part of this PR?

@antross yes please!

@molant

Couple minor things. I'll review it more in depth later. Nice job 👏

@@ -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;

This comment has been minimized.

@molant

molant Sep 7, 2018

Member

Can you please add the types back? We are trying to go full strict (#576) and I think this is needed.

This comment has been minimized.

@antross

antross Sep 7, 2018

Member

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. :)

This comment has been minimized.

@molant

molant Sep 7, 2018

Member

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...

@@ -170,7 +171,7 @@ export default class ManifestParser extends Parser {
* is a valid acording to the schema.
*/
const validationResult: SchemaValidationResult = validate(this.schema, jsonContent);
const validationResult = validate(this.schema, result.data, result.getLocation);

This comment has been minimized.

@molant

molant Sep 7, 2018

Member

Can you add a type here?

@sarvaje

sarvaje approved these changes Sep 7, 2018

LGTM. Just the same comments as @molant

@@ -55,7 +55,7 @@ const tests: Array<HintLocalTest> = [
name: 'If .babelrc contains an invalid extends, it should fail',
path: path.join(__dirname, 'fixtures', 'invalid-extends'),
reports: [
{ message: `Unexpected token ' in JSON at position 191` }
{ message: `Unexpected token ' in JSON at position 202` }

This comment has been minimized.

@antross

antross Sep 10, 2018

Member

@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.

This comment has been minimized.

@molant

molant Sep 10, 2018

Member

Maybe you are checking out your files with \r\n in your machine?
What do you think of:

  • Make sure those filed don't have \r\n
  • Change your options to "Checkout as-is, commit Unix-style": 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

This comment has been minimized.

@antross

antross Sep 10, 2018

Member

I fixed this by adding eol=lf to .gitattributes in 3a58198. This ensures files are checked out with LF 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).

This comment has been minimized.

@alrra

alrra Sep 10, 2018

Member

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.

This comment has been minimized.

@antross

antross Sep 10, 2018

Member

My recommendation is to not use eol=lf as it will bite users in the future.

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.

This comment has been minimized.

@molant

molant Sep 11, 2018

Member

My recommendation is to not use eol=lf as it will bite users in the future.

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.

This comment has been minimized.

@alrra

alrra Sep 11, 2018

Member

All Windows editors (including notepad) support this line ending and not having this option has already bite us.

Ok.

This comment has been minimized.

@alrra

alrra Sep 11, 2018

Member

I'll take that as we're keeping eol=lf

Yes.

antross added some commits Sep 10, 2018

Chore: Always checkout files with LF endings (so tests work)
The `* text=auto` attribute already ensured all commits were
converted to `LF` in the repository, but depending on user settings
files could still be checked out with `CRLF` on Windows (which
breaks some of our test cases). Setting the `eol=lf` attribute ensures
files are always checked out with `LF` even on Windows regardless
of user settings.
Chore: Revert incorrect change to error position
This was happening due to a CRLF issue on Windows.
@molant

molant approved these changes Sep 10, 2018

@antross

This comment has been minimized.

Member

antross commented Sep 10, 2018

Don't know why travis-ci disappeared from the checks, but the last build passed:
https://travis-ci.org/webhintio/hint/builds/426836878

@molant

This comment has been minimized.

Member

molant commented Sep 10, 2018

@alrra do you want to take a look or are you ok?

@antross

This comment has been minimized.

Member

antross commented Sep 11, 2018

All Windows editors (including notepad) support this line ending and not having this option has already bite us.

Ok.

@alrra I'll take that as we're keeping eol=lf. Anything else you need from me on this PR?

@webhintio webhintio deleted a comment from molant Sep 11, 2018

@alrra alrra closed this in 0766455 Sep 11, 2018

@alrra

This comment has been minimized.

Member

alrra commented Sep 11, 2018

@antross New versions of the packages including these changes are now published! 🎉

@antross

This comment has been minimized.

Member

antross commented Sep 11, 2018

@alrra Awesome, thanks! 🎉

molant added a commit to molant/hint that referenced this pull request Sep 12, 2018

@antross antross deleted the antross:utils-parse-json branch Oct 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment