-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
#2025: Add check to ensure files end in .json #2026
base: master
Are you sure you want to change the base?
Conversation
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.
Thank you very much! Funny that I did the same mistake over a year ago.. :D
message: `File \`${filename}\` should end in .json. Please rename the file.`, | ||
location: { start: { line: 1 } }, |
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.
This won't make much sense as a suggestion from the reviewbot, but afaik there is no way for a code suggestion to rename a file in GH. @baltpeter do you have an opinion on this?
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.
afaik there is no way for a code suggestion to rename a file in GH.
No, there isn't. I agree, this should just be a comment without a suggestion.
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.
Thank you! (And sorry for taking so long with the review.)
genericlint: { | ||
checks: await importChecks('generic'), | ||
url: 'https://github.com/datenanfragen/data/blob/master/src/checks/generic', | ||
path_filter: (path: string) => path.startsWith('companies/') || path.startsWith('company-packs/') || path.startsWith('supervisory-authorities/') || path.startsWith('suggested-companies/'), | ||
}, |
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.
Do we need to establish another check type for this? The suggested companies are deprecated (and can actually be removed from the repo now). If we made this a record
check, we would indeed miss the company packs, but I feel like this is less critical for them (a missing company pack is a lot more obvious than a missing company). What do you think?
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.
Actually: If we decide, we only need this for record
s, do we still need the additional check at all? I think match-filename-slug
should cover this already in that case.
@@ -37,7 +37,7 @@ const validate = async (dir: string) => { | |||
const check_results: RdjsonLine[] = []; | |||
|
|||
const linters = await _linters(); | |||
const files = glob.sync(`${dir}/*.json`); | |||
const files = glob.sync(`${dir}/*`); |
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 worried this might create a problem on systems that like to automatically place files in directories (Thumbs.db
, .DS_Store
etc.).
We should ignore everything in the .gitignore
in this matcher. Unfortunately, our glob
library doesn't support that as far as I can see. It might be worth switching to something like https://github.com/sindresorhus/globby.
@@ -0,0 +1,22 @@ | |||
import { basename, extname } from 'path'; | |||
import { exit } from 'process'; |
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.
This looks like a stray import.
const filename = basename(ctx.file_path); | ||
const file_extension = extname(ctx.file_path) | ||
|
||
if (file_extension != ".json") |
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.
if (file_extension != ".json") | |
if (file_extension !== ".json") |
message: `File \`${filename}\` should end in .json. Please rename the file.`, | ||
location: { start: { line: 1 } }, |
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.
afaik there is no way for a code suggestion to rename a file in GH.
No, there isn't. I agree, this should just be a comment without a suggestion.
This should help prevent errors from manually editing within the github browser :^)
Closes #2025