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

#2025: Add check to ensure files end in .json #2026

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion companies/autoscout24.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"insurance"
],
"name": "AutoScout24 GmbH",
"address": "Bothestraße 11 – 15\n81675 München\nDeutschland",
"address": "Bothestraße 11 – 15\n81675 München\nDeutschland",
"phone": "+49 89 444 56 1666",
"email": "[email protected]",
"web": "https://www.autoscout24.de/",
Expand Down
File renamed without changes.
2 changes: 1 addition & 1 deletion companies/moebel-boss.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"commerce"
],
"name": "SB-Möbel Boss Handelsgesellschaft mbH & Co. KG",
"address": "Bakenweg 16 – 20\n32457 Porta Westfalica\nDeutschland",
"address": "Bakenweg 16 – 20\n32457 Porta Westfalica\nDeutschland",
"phone": "+49 5731 609 0",
"fax": "+49 5731 609 588",
"email": "[email protected]",
Expand Down
2 changes: 1 addition & 1 deletion companies/prettylittlething.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"commerce"
],
"name": "Prettylittlething.com Ltd.",
"address": "49 – 51 Dale Street\nManchester\nM1 2HF\nUnited Kingdom",
"address": "49 – 51 Dale Street\nManchester\nM1 2HF\nUnited Kingdom",
"email": "[email protected]",
"web": "https://www.prettylittlething.com",
"sources": [
Expand Down
2 changes: 1 addition & 1 deletion companies/soundcloud.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"SoundCloud Go+",
"SoundCloud Inc."
],
"address": "Rheinsberger Straße 76 – 77\n10115 Berlin\nDeutschland",
"address": "Rheinsberger Straße 76 – 77\n10115 Berlin\nDeutschland",
"phone": "+49 30 467 247 600",
"email": "[email protected]",
"web": "https://soundcloud.com/",
Expand Down
22 changes: 22 additions & 0 deletions src/checks/generic/json-file-extension.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { basename, extname } from 'path';
import { exit } from 'process';
Copy link
Member

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.

import { Check } from '../../types/checks';

const check: Check = {
id: 'json-file-extension',
desc: 'Data files should end with `.json` (#2025)',
url: 'https://github.com/datenanfragen/data/#data-formats',
severity: 'ERROR',
run: (json, ctx) => {
const filename = basename(ctx.file_path);
const file_extension = extname(ctx.file_path)

if (file_extension != ".json")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (file_extension != ".json")
if (file_extension !== ".json")

return {
message: `File \`${filename}\` should end in .json. Please rename the file.`,
location: { start: { line: 1 } },
Comment on lines +16 to +17
Copy link
Member

@mal-tee mal-tee Dec 31, 2022

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?

Copy link
Member

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.

suggestions: [basename(filename, file_extension) + '.json'],
};
},
};
export default check;
5 changes: 5 additions & 0 deletions src/checks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ const linters = async () => {
url: 'https://github.com/datenanfragen/data/blob/master/src/checks/pack',
path_filter: (path: string) => path.startsWith('company-packs/'),
},
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/'),
},
Comment on lines +35 to +39
Copy link
Member

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?

Copy link
Member

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 records, do we still need the additional check at all? I think match-filename-slug should cover this already in that case.

};
};
export default linters;
4 changes: 2 additions & 2 deletions src/checks/record/match-filename-slug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ const check: Check = {
url: 'https://github.com/datenanfragen/data/#data-formats',
severity: 'ERROR',
run: (json, ctx) => {
const filename = basename(ctx.file_path);
if (json.slug + '.json' !== filename)
const filename = basename(ctx.file_path, '.json');
if (json.slug !== filename)
return {
message: `Filename \`${filename}\` does not match slug \`${json.slug}\`.

Expand Down
2 changes: 1 addition & 1 deletion src/test-records.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}/*`);
Copy link
Member

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.


for (const f of files) {
const file_content = fs.readFileSync(f).toString();
Expand Down
2 changes: 1 addition & 1 deletion supervisory-authorities/grdpa.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"gr"
],
"name": "Αρχή Προστασίας Δεδομένων Προσωπικού Χαρακτήρα (Hellenic Data Protection Authority)",
"address": "Kifissias 1 – 3\n115 23 Athens\nGreece",
"address": "Kifissias 1 – 3\n115 23 Athens\nGreece",
"phone": "+30 210 64756 00",
"fax": "+30 210 64756 28",
"email": "[email protected]",
Expand Down