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.

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

Meta: Fix feature details in Options page #2219

Merged
merged 7 commits into from Jul 11, 2019

Conversation

Projects
None yet
3 participants
@sharkykh
Copy link
Contributor

commented Jul 9, 2019

I'm still fairly new to TypeScript, so hopefully this is okay.

Fixes 672793e#r34235994:

[...] unfortunately this code does not work as intended.
It only uses the first match it finds, so for example, on default-to-rich-diff.tsx, disabled is matched first, and so description is undefined.

Also fixes the missing screenshot links.

With this PR:

Show resolved Hide resolved webpack.config.ts Outdated
@bfred-it
Copy link
Collaborator

left a comment

My ideal fix would be to move this meta data to a comment in the file header, it's easier and safer to parse as plain text. e.g.

/*
Insert collapsible content when writing comments (via `<summary>`)

https://user-images.githubusercontent.com/1402241/53678019-0c721280-3cf4-11e9-9c24-4d11a697f67c.png
*/
Show resolved Hide resolved webpack.config.ts Outdated
Show resolved Hide resolved webpack.config.ts
@sharkykh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

My ideal fix would be to move this meta data to a comment in the file header, it's easier and safer to parse as plain text. e.g.

/*
Insert collapsible content when writing comments (via `<summary>`)

https://user-images.githubusercontent.com/1402241/53678019-0c721280-3cf4-11e9-9c24-4d11a697f67c.png
*/

How would you tell disabled, screenshot and description apart in this format?

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

description is first line
screenshot is the URL (edit: specifically a URL on the second line)
disabled would still be in the features.add field

@jerone

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@bfred-it commented on Jul 10, 2019, 9:16 AM GMT+2:

description is first line
screenshot is the URL
disabled would still be in the features.add field

Or we implement something similar to userscript metadata block:

// ==UserScript==
// @key value
// ==/UserScript==

Information about this:

We probably only need a few right now:

// ==UserScript==
// @description		Insert collapsible content when writing comments (via `<summary>`)
// @screenshotUrl	https://user-images.githubusercontent.com/1402241/53678019-0c721280-3cf4-11e9-9c24-4d11a697f67c.png
// @author			@bfred-it
// @author			@jerone
// ==/UserScript==

Later we can extend that with @version, translations (@description:XX-YY), on which page to load (@include and @exclude), an icon (@iconURL), etc...

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

The point of using a header comment is to avoid having parsers for it. Currently description and screenshot is all we need, and there's not much needed to parse it. Even if we wanted to add authors (which we don't, because we have git history) we'd just append @bfred-it @jerone which is equally easy to parse.

This metadata only used for options; The other fields you describe are used at runtime and we already have them in features.add's object

@bfred-it bfred-it added the meta label Jul 10, 2019

but it should be
${field}: '${validValue}'
`);

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 11, 2019

Collaborator

Error examples:

                                throw new Error(`
          ^
Error: 
Invalid characters found in `swap-branches-on-compare`’s disabled field. It’s

disabled: ' https://user-images.githubusercontent.com/1402241/51669708-9a712400-1ff7-11e9-913a-ac1ea1050975.png '

but it should be

disabled: 'https://user-images.githubusercontent.com/1402241/51669708-9a712400-1ff7-11e9-913a-ac1ea1050975.png'

                                throw new Error(`
          ^
Error: 
Invalid characters found in `swap-branches-on-compare`’s description field. It’s

description: ' Swap head and base branches\' in the branch compare view'

but it should be

description: 'Swap head and base branches’ in the branch compare view'

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 11, 2019

Collaborator

Shortened to

Invalid characters found in `swap-branches-on-compare`. Apply this patch:

- description: ' Swap head and base branches\' in the branch compare view'
+ description: 'Swap head and base branches’ in the branch compare view'

bfred-it added some commits Jul 11, 2019

@bfred-it bfred-it changed the title Meta: Fix feature details extraction Meta: Fix feature details in Options page Jul 11, 2019

@sharkykh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@bfred-it I wish I had thought about the solution you just pushed. Much cleaner, much simpler. 👍


Continuing the discussion about the header comment:

I think having a comment block with plain text (I mean without any type of key: value structure), while fairly easy to parse (compared to using regular expressions), could also be a bit fragile, so you might forget a new line, and now the feature has no screenshot even though it's in the file.

Furthermore,

description is first line
screenshot is the URL (edit: specifically a URL on the second line)
disabled would still be in the features.add field

You still need disabled for the options page (for __featuresInfo__).
If you leave it as a features.add field, you still need to parse the file for it... Right?

@bfred-it bfred-it merged commit 73bef2d into sindresorhus:master Jul 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sharkykh sharkykh deleted the sharkykh:fix-parse-feature-details branch Jul 11, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

Merging this because it's a bugfix. We can continue the discussion for any possible future changes

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

If you leave it as a features.add field, you still need to parse the file for it... Right?

Right. The main reason for the move would be that description and screenshot are never part of the runtime. Heck technically instead of a disabled property we should exclude the file from webpack, but keeping it there as a property is:

  • easy to implement
  • easy to exclude at runtime

a comment block with plain text ... could also be a bit fragile

Indeed it is, so is the current method of trying to parse JS with a regex. I don't really want to load up Acorn just for this, so here were are with some simple regexes and some throw new Errors.

I think parsing that header comment block is relatively easy:

const [, meta] = /^[/][*]([^]*)[*][/]/.exec(fileContent);
if(!meta) {
	throw new Error(`Description not found. Follow contributors.md`)
}
const [description, screenshot] = meta.trim().split('\n').filter(Boolean);
if(screenshot && !/^https:/.test(screenshot)) {
	throw new Error(`Screenshot invalid: ${screenshot}`)
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.