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

lint: add initial checks for approaches and articles #704

Merged
merged 13 commits into from
Nov 11, 2022

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Nov 4, 2022

There is not yet a formal spec for an approach config.json file or an article config.json file, but I've tried to implement linting them from the docs for concept exercises and practice exercises (which talk about approaches) and a commit on the csharp track.

There is not yet a formal spec for an approach `config.json` file or an
article `config.json` file, but I've tried to implement linting them
from the docs for approaches [1][2] and a commit [3] on the csharp
track.

[1] https://github.com/exercism/docs/blob/5509b2f12fac/building/tracks/concept-exercises.md#file-approachesconfigjson
[2] https://github.com/exercism/docs/blob/5509b2f12fac/building/tracks/practice-exercises.md#file-approachesconfigjson
[3] exercism/csharp@4069cca97782
@ee7 ee7 requested a review from ErikSchierboom as a code owner November 4, 2022 16:28
Comment on lines 6 to 8
DirKind = enum
dkApproaches = "approaches"
dkArticles = "articles"
Copy link
Member Author

Choose a reason for hiding this comment

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

DirKind isn't a great name. Do we have a name for something that is an approach or article?

Copy link
Member

Choose a reason for hiding this comment

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

They are part of the Dig Deeper section, maybe something like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do something like DigDeeperKind, but then dots in the enum string representations are bad.

If this is blocking, please suggest a name.

Copy link
Member

Choose a reason for hiding this comment

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

DigDeeperDirKind? That's bad too. This is not blocking

src/lint/approaches_and_articles.nim Outdated Show resolved Hide resolved
src/lint/approaches_and_articles.nim Outdated Show resolved Hide resolved
ee7 added 2 commits November 4, 2022 19:03
Make `configlet lint` produce an error for a snippet with 8 newline
characters then non-newline content on the 9th line, e.g.:

    1\n2\n3\n4\n5\n6\n7\n8\n9

Note that this means we do not count lines like `wc`:

    $ printf '1\n2\n3\n4\n5\n6\n7\n8' | wc -l
    7
    $ printf '1\n2\n3\n4\n5\n6\n7\n8\n' | wc -l
    8
    $ printf '1\n2\n3\n4\n5\n6\n7\n8\n9' | wc -l
    8
    $ printf '1\n2\n3\n4\n5\n6\n7\n8\n9\n' | wc -l
    9
@ee7 ee7 requested a review from ErikSchierboom November 9, 2022 12:41
result = 0
if s.len > 0:
for line in s.splitLines():
if not (line.startsWith("```") and dk == dkArticles):
Copy link
Member Author

Choose a reason for hiding this comment

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

Does not handle the fact that a code fence can begin with ~~~

Copy link
Member

Choose a reason for hiding this comment

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

That's probably perfectly fine

@ErikSchierboom
Copy link
Member

We currently output:

- Every practice exercise has the required .md files
- Every practice exercise has a valid .meta/config.json file

Would it perhaps be useful here to also add that we're checking articles and approaches?

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.

Lovely work! Some minor nits

src/lint/approaches_and_articles.nim Show resolved Hide resolved
src/lint/approaches_and_articles.nim Show resolved Hide resolved
src/lint/approaches_and_articles.nim Outdated Show resolved Hide resolved
result = 0
if s.len > 0:
for line in s.splitLines():
if not (line.startsWith("```") and dk == dkArticles):
Copy link
Member

Choose a reason for hiding this comment

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

That's probably perfectly fine

@ee7 ee7 requested a review from ErikSchierboom November 10, 2022 15:37
@ee7
Copy link
Member Author

ee7 commented Nov 10, 2022

I'll PR for the errors in csharp and rust.

Edit:

At the time of writing, the only other tracks with approaches/articles in main are javascript and ruby. For those, a configlet built from the HEAD of this PR's branch does not produce a linting error.

@ee7
Copy link
Member Author

ee7 commented Nov 10, 2022

We currently output:

- Every practice exercise has the required .md files
- Every practice exercise has a valid .meta/config.json file

Would it perhaps be useful here to also add that we're checking articles and approaches?

OK - 6747ce1. Feel free to bikeshed the wording.

In general I don't love the summary message, though. I've considered linking to #249.

Things would be better with a refactor that defines all the implemented checks and their error messages in one places.

@ErikSchierboom
Copy link
Member

In general I don't love the summary message, though. I've considered linking to #249.

Things would be better with a refactor that defines all the implemented checks and their error messages in one places.

I agree. That would be a lot more convenient.

@ErikSchierboom
Copy link
Member

I've found that for both approaches and articles, the files within those directory are not checked. I could rename exercises/practice/leap/.approaches/boolean-chain/content.md to exercises/practice/leap/.approaches/boolean-chain/content.md2 and not get an error message. Same with the snippet file.

@ee7
Copy link
Member Author

ee7 commented Nov 11, 2022

I've found that for both approaches and articles, the files within those directory are not checked. I could rename exercises/practice/leap/.approaches/boolean-chain/content.md to exercises/practice/leap/.approaches/boolean-chain/content.md2 and not get an error message. Same with the snippet file.

Ok. Is the below the logic that we want in the spec?

.approaches/config.json

  • If the introduction.authors array is non-empty, there must be a non-empty introduction.md file
  • Any approaches.slug value must have a corresponding non-empty <slug>/content.md file
  • Any approaches.slug value must have a corresponding non-empty <slug>/snippet.txt file

.articles/config.json

  • Any articles.slug value must have a corresponding non-empty <slug>/content.md file
  • Any articles.slug value must have a corresponding non-empty <slug>/snippet.md file

@ErikSchierboom
Copy link
Member

If the introduction.authors array is non-empty, there must be a non-empty introduction.md file

I'd make this more strict and have it be:

If the introduction object is present, there must be a non-empty introduction.md file

@ErikSchierboom
Copy link
Member

But yes, that sounds good!

ee7 added 2 commits November 11, 2022 13:58
Try to implement these checks for `.approaches/config.json`

- If the `introduction.authors` array is non-empty, there must be a non-empty `introduction.md` file
- Any `approaches.slug` value must have a corresponding non-empty `<slug>/content.md` file
- Any `approaches.slug` value must have a corresponding non-empty `<slug>/snippet.txt` file

And these checks for `.articles/config.json`

- Any `articles.slug` value must have a corresponding non-empty `<slug>/content.md` file
- Any `articles.slug` value must have a corresponding non-empty `<slug>/snippet.md` file

Some output:

    $ configlet lint
    [...]
    The config.json 'introduction' object is present, but there is no corresponding introduction file at the below location:
    ./exercises/practice/bob/.approaches/introduction.md

    A config.json 'approaches.slug' value is 'if', but there is no corresponding content file at the below location:
    ./exercises/practice/bob/.approaches/if/content.md

    A config.json 'approaches.slug' value is 'answer-array', but there is no corresponding snippet file at the below location:
    ./exercises/practice/bob/.approaches/answer-array/snippet.txt

    A config.json 'articles.slug' value is 'performance', but there is no corresponding content file at the below location:
    ./exercises/practice/bob/.articles/performance/content.md

    A config.json 'articles.slug' value is 'performance', but there is no corresponding snippet file at the below location:
    ./exercises/practice/bob/.articles/performance/snippet.md
@ee7
Copy link
Member Author

ee7 commented Nov 11, 2022

Please take another look.

Sample output:

$ configlet lint
[...]
The config.json 'introduction' object is present, but there is no corresponding introduction file at the below location:
./exercises/practice/bob/.approaches/introduction.md

A config.json 'approaches.slug' value is 'if', but there is no corresponding content file at the below location:
./exercises/practice/bob/.approaches/if/content.md

A config.json 'approaches.slug' value is 'answer-array', but there is no corresponding snippet file at the below location:
./exercises/practice/bob/.approaches/answer-array/snippet.txt

A config.json 'articles.slug' value is 'performance', but there is no corresponding content file at the below location:
./exercises/practice/bob/.articles/performance/content.md

A config.json 'articles.slug' value is 'performance', but there is no corresponding snippet file at the below location:
./exercises/practice/bob/.articles/performance/snippet.md

@ErikSchierboom
Copy link
Member

See https://github.com/exercism/docs/pull/406/files for an attempt at defining the linting rules

@ErikSchierboom
Copy link
Member

Sample output:

That looks great! The only thing that I'm missing is that if there is a sub-directory for an article or approach but no entry in the config.json file, that there is no error.

Example output:

    $ configlet lint
    [...]
    There is no 'approaches.slug' key with the value 'performance', but a sibling directory exists with that name:
    ../exercism-tracks/csharp/exercises/practice/reverse-string/.approaches/config.json
@ee7
Copy link
Member Author

ee7 commented Nov 11, 2022

The only thing that I'm missing is that if there is a sub-directory for an article or approach but no entry in the config.json file, that there is no error.

OK - please take another look.

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.

Love it! Any linting issues for C#/Rust/JS?

@ee7
Copy link
Member Author

ee7 commented Nov 11, 2022

Love it! Any linting issues for C#/Rust/JS?

No, nor for Ruby. Nor for open PRs that add articles/approaches on those tracks.

However, I have not run configlet lint on any PR that adds an approach/article to a track that currently has neither. That means that a PR:

  • for which the final check ran before the configlet 4.0.0-beta.8 release
  • but was merged after that release

would make the configlet lint check indicate an error on the commit merged to main.

@ErikSchierboom
Copy link
Member

I think that's fine. We'll fix them later

@ee7 ee7 merged commit 8907279 into exercism:main Nov 11, 2022
@ee7 ee7 deleted the lint-approaches-and-articles branch November 11, 2022 17:37
@ee7 ee7 changed the title lint: check approaches and articles lint: add initial checks for approaches and articles Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants