Skip to content

Conversation

@troyciesco
Copy link
Contributor

ref #573

  • currently when a file has multiple custom theme setting errors, gscan only reports one at a time. This makes it difficult to debug a file if there are multiple errors.
  • with this change, all errors for a file will be shown with a line number to make it easy to edit
  • using the cli without the verbose tag still shows a comma-delimited list, with each file listed only once

before:
Screenshot 2025-05-03 at 10 42 21 AM

after (each file only has two errors, but i did test with 1 and 3):
Screenshot 2025-05-03 at 11 25 27 AM

cli output without --verbose:

Your theme has 1 error!
----

Errors
------
Important to fix, functionality may be degraded.

- Error: An unknown custom theme setting has been used.
Affected Files: index.hbs, partials/post-card.hbs, page.hbs, post.hbs

  • The smaller footprint of this PR makes it easier to find and fix bugs in theme files without completely overhauling the checker

ref TryGhost#573
- makes it easier to find and fix bugs in theme files without completely overhauling the checker
theme.results.error.byFiles['assets/my.css'].length.should.eql(3);
theme.results.error.byFiles['default.hbs'].length.should.eql(21);
theme.results.error.byFiles['post.hbs'].length.should.eql(65);
theme.results.error.byFiles['post.hbs'].length.should.eql(66);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updating this test here makes sense, because before only @price.monthly was logged, but now @price.yearly is as well. File here: https://github.com/TryGhost/gscan/blob/main/test/fixtures/themes/001-deprecations/v5/invalid_all/post.hbs#L5

I don't think there are additional tests to add but I can if needed!

});
} else {
ui.log(`${chalk.bold('Affected Files:')} ${_.map(result.failures, 'ref')}`);
ui.log(`${chalk.bold('Affected Files:')} ${_.uniq(_.map(result.failures, 'ref')).join(', ')}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer this output:

Affected Files: index.hbs, partials/post-card.hbs, page.hbs, post.hbs

to this:

Affected Files: index.hbs,partials/post-card.hbs,page.hbs,post.hbs

but if there was a reason for leaving out the spaces i can certainly switch it back.

I do think _.uniq is useful, because otherwise it looks like this:

Affected Files: index.hbs,index.hbs,partials/post-card.hbs,partials/post-card.hbs,page.hbs,page.hbs,post.hbs,post.hbs

Although, now I'm wondering if there's ever a reason you'd want duplicated filenames for different failure results that are output by this function - probably not? But can change if needed!

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.

1 participant