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(track_config): check key_features #236

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Mar 24, 2021

This commit implements the below rules for a track's root config.json
file.

  • The "key_features" key is optional
  • The "key_features" value must be an array with length = 6
  • The "key_features[].title" key is required
  • The "key_features[].title" value must be a non-empty, non-blank
    string with length <= 25
  • The "key_features[].content" key is required
  • The "key_features[].content" value must be a non-empty, non-blank
    string with length <= 100

These rules are currently disabled until we have a list of valid icons:

  • The "key_features[].icon" key is required
  • The "key_features[].icon" value must be a string that matches one of
    the pre-defined icon values

See the spec:


Questions for @ErikSchierboom:

  1. The string here has a rune length of 100, but a len of 102 (it contains , which is 3 bytes). Should we be checking for length or rune length?
  2. Should we hide the icon errors for now?

This PR produces the following diff to the output of configlet lint, per track:

common-lisp

+The value of `content` that starts with `The language is very stabl...` is 102 characters, but must not exceed 100 characters:
+./config.json
+
+Configlet detected at least one problem.
+For more information on resolving the problems, please see the documentation:
+https://github.com/exercism/docs/blob/main/building/configlet/lint.md

julia

+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+

lua

+String is zero-length: 'icon':
+./config.json
+
+The value of `content` that starts with `Lua is primarily designed ...` is 167 characters, but must not exceed 100 characters:
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+The value of `content` that starts with `Lua is known to be one of ...` is 149 characters, but must not exceed 100 characters:
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+The value of `content` that starts with `Lua has a simple, yet powe...` is 112 characters, but must not exceed 100 characters:
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+The value of `content` that starts with `Lua offers a small, yet fl...` is 133 characters, but must not exceed 100 characters:
+./config.json
+
+Configlet detected at least one problem.
+For more information on resolving the problems, please see the documentation:
+https://github.com/exercism/docs/blob/main/building/configlet/lint.md

php

+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+Configlet detected at least one problem.
+For more information on resolving the problems, please see the documentation:
+https://github.com/exercism/docs/blob/main/building/configlet/lint.md

python

+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+String is zero-length: 'icon':
+./config.json
+
+Configlet detected at least one problem.
+For more information on resolving the problems, please see the documentation:
+https://github.com/exercism/docs/blob/main/building/configlet/lint.md

rust

+Missing key: 'icon':
+./config.json
+
+Missing key: 'icon':
+./config.json
+
+Missing key: 'icon':
+./config.json
+
+Missing key: 'icon':
+./config.json
+
+Missing key: 'icon':
+./config.json
+
+Missing key: 'icon':
+./config.json
+
+Configlet detected at least one problem.
+For more information on resolving the problems, please see the documentation:
+https://github.com/exercism/docs/blob/main/building/configlet/lint.md

@ErikSchierboom
Copy link
Member

The string here has a rune length of 100, but a len of 102 (it contains –, which is 3 bytes). Should we be checking for length or rune length?

It is the number of runes.

Should we hide the icon errors for now?

We should

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

This commit implements the below rules for a track's `config.json` file.
- The `"key_features"` key is optional
- The `"key_features"` value must be an array with length = 6
- The `"key_features[].title"` key is required
- The `"key_features[].title"` value must be a non-empty, non-blank
  string with length <= 25
- The `"key_features[].content"` key is required
- The `"key_features[].content"` value must be a non-empty, non-blank
  string with length <= 100

These rules are currently disabled until we have a list of valid icons:
- The `"key_features[].icon"` key is required
- The `"key_features[].icon"` value must be a string that matches one of
  the pre-defined icon values

See the spec:
- https://github.com/exercism/docs/blob/main/building/configlet/lint.md#rule-configjson-file-is-valid
@ee7 ee7 force-pushed the lint-track-config-key-features branch from 323ebee to c9acd22 Compare March 25, 2021 11:45
@ee7
Copy link
Member Author

ee7 commented Mar 25, 2021

Current diff due to this PR:

lua

Ideally the below should say key_features.content, but that will have to wait for some later refactoring.

+The value of `content` that starts with `Lua is primarily designed...` is 167 characters, but must not exceed 100 characters:
+./config.json
+
+The value of `content` that starts with `Lua is known to be one of...` is 149 characters, but must not exceed 100 characters:
+./config.json
+
+The value of `content` that starts with `Lua has a simple, yet pow...` is 112 characters, but must not exceed 100 characters:
+./config.json
+
+The value of `content` that starts with `Lua offers a small, yet f...` is 133 characters, but must not exceed 100 characters:
+./config.json
+
+Configlet detected at least one problem.
+For more information on resolving the problems, please see the documentation:
+https://github.com/exercism/docs/blob/main/building/configlet/lint.md

@ee7
Copy link
Member Author

ee7 commented Mar 25, 2021

@ErikSchierboom Non-blocking for this PR, but somewhat relevant: currently, we do not allow an optional string to be the empty string. Is that correct? We only allow it to be missing or null.

The current code is below. At the bottom, we have else not elif isRequired.

of JString:
let s = data[key].getStr()
if s.len > 0:
if s.strip().len > 0:
if not hasValidRuneLength(s, key, path, maxLen):
result = false
else:
result.setFalseAndPrint("String is whitespace-only: " & q(key), path)
else:
result.setFalseAndPrint("String is zero-length: " & q(key), path)

@ErikSchierboom
Copy link
Member

currently, we do not allow an optional string to be the empty string. Is that correct? We only allow it to be missing or null.

Hmmm. For now, I'd leave it like that. I do find that an empty string is something else than omitting a value or using null. We might revisit later, but for now this is good I think.

@ee7 ee7 merged commit fb47121 into exercism:main Mar 25, 2021
@ee7 ee7 deleted the lint-track-config-key-features branch March 25, 2021 12:29
@ee7 ee7 mentioned this pull request Mar 26, 2021
ee7 added a commit that referenced this pull request Mar 26, 2021
This commit adds `status`, `online_editor`, and `key_features` to the
success message.

These changes should have been included in:
- 3d19f64 (#235)
- fb47121 (#236)
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