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

Add files linting rules for track-level config.json #155

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Jun 21, 2021

These properties were described in building/tracks/config-json.md, but
were missing from building/configlet/lint.md.


Do we want this?

It's true that the track-level files property will only used by configlet sync. But it feels like configlet lint should complain about a problem in this property, otherwise a user might:

  1. Clone a track
  2. Run configlet sync
  3. See an error message due to an invalid pattern in the files property

The alternative attempt to catch a problem with files at CI-time doesn't work: we could run configlet sync during CI on every track, and make it exit non-zero for a problem with files. But configlet sync will already produce a non-zero exit code if there's something unsynced, so we can't easily distinguish a problem with files.

@iHiD iHiD requested a review from ErikSchierboom June 21, 2021 10:56
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Thanks! It was just an omission on my part, they should definitely have been documented.

building/configlet/lint.md Show resolved Hide resolved
building/configlet/lint.md Show resolved Hide resolved
building/configlet/lint.md Show resolved Hide resolved
building/configlet/lint.md Show resolved Hide resolved
building/configlet/lint.md Show resolved Hide resolved
building/configlet/lint.md Show resolved Hide resolved
building/configlet/lint.md Show resolved Hide resolved
building/configlet/lint.md Show resolved Hide resolved
building/configlet/lint.md Show resolved Hide resolved
@ee7
Copy link
Member Author

ee7 commented Jun 22, 2021

@ErikSchierboom So the files property in the track config.json is required?

If yes, this PR should update building/tracks/config-json.md too, where it's documented as optional:

- `files`: The patterns for the locations of the files used in an exercise, relative to the exercise's directory. (optional)
- `solution`: stub implementation file(s) pattern (optional)
- `test`: test file(s) pattern (optional)
- `example`: example implementation file(s) pattern (optional
- `exemplar`: exemplar implementation file(s) pattern (optional)
- `editor`: additional read-only editor file(s) patterns (optional)

@ErikSchierboom
Copy link
Member

Oh noes, I've completely misread :( Ignore all my comments.

@ee7
Copy link
Member Author

ee7 commented Jun 22, 2021

So what are your real thoughts? :)

I was thinking that I'd make configlet sync --filepaths first run the same filepath checking code as configlet lint.

@ErikSchierboom
Copy link
Member

I was thinking that I'd make configlet sync --filepaths first run the same filepath checking code as configlet lint.

What I would expect configlet sync --filepaths to do is:

  1. Looks for any exercises where their files.solution/files.test/files.example/files.examplar property is an empty array
  2. For those empty file arrays, check if the track's config.json file has a corresponding file path pattern defined.
  3. If such a pattern is defined, replace the empty property with the proper value for this exercise based on the file path pattern

And that's it. I think I'd prefer the actual linting of the file paths to happen only when configlet lint runs, just to keep the functionality nice and separated.

@ee7
Copy link
Member Author

ee7 commented Jun 22, 2021

So just to confirm: do you want an informational message if e.g. files.solution is non-empty, but no element matches the pattern? (Seems like the answer is no).

What I would expect configlet sync --filepaths to do is:

Do you mean configlet sync --filepaths --update?

  1. Looks for any exercises where their files.solution/files.test/files.example/files.examplar property is an empty array

What should configlet sync --filepaths --update do if it sees an exercise config.json where

  • the files property is missing entirely?
  • the files property is present, but is not an object?
  • the files property is present, and is an object, but contains only one property?

@ErikSchierboom
Copy link
Member

So just to confirm: do you want an informational message if e.g. files.solution is non-empty, but no element matches the pattern? (Seems like the answer is no).

No, I'd only display the information message when running configlet lint

Do you mean configlet sync --filepaths --update?

I do. Forgot that we're now non-interactive by default :)

What should configlet sync --filepaths --update do if it sees an exercise config.json where
the files property is missing entirely?

I would like the files property to be added with values populated based on the file path patterns in the config.json. The exception to this would be if there are no file path patterns defined, in which case I don't think we should do anything.

the files property is present, but is not an object?

Probably output an error.

the files property is present, and is an object, but contains only one property?

If that property is empty and there is a file path pattern defined for it: populate it based on the pattern. For the other properties: if there are file path patterns defined for them, I think would just add those too.

These properties were described in `building/tracks/config-json.md`, but
were missing from `building/configlet/lint.md`.
@ee7 ee7 force-pushed the linting-rules-add-files-property branch from 7bef302 to 3b4d965 Compare July 7, 2021 16:42
@ee7
Copy link
Member Author

ee7 commented Jul 7, 2021

Resolved the merge conflicts from b98257f.

@ErikSchierboom I side-tracked this PR into a configlet sync discussion. But is this PR correct, and what we want?

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

LGTM!

@ErikSchierboom ErikSchierboom merged commit 37e2720 into exercism:main Jul 22, 2021
@ee7 ee7 deleted the linting-rules-add-files-property branch July 22, 2021 07:44
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.

2 participants