Skip to content

feat: general fixes#196

Open
SecondSkoll wants to merge 12 commits intomainfrom
general-fixes
Open

feat: general fixes#196
SecondSkoll wants to merge 12 commits intomainfrom
general-fixes

Conversation

@SecondSkoll
Copy link
Copy Markdown
Contributor

  • Adds date format rule
  • Better string capture for release name rule context
  • Update docs link
  • Additional protections for inline code strings
  • Adds dictionary terms
  • Refines prompt rule to ensure captured element extends to next word to prevent bug with specified location
  • Adds surrounding characters to inline comment rule - to ensure symbols are not flagged erroneously

Copy link
Copy Markdown
Contributor

@dwilding dwilding left a comment

Choose a reason for hiding this comment

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

Thanks! A few questions and minor suggestions

Anbox Cloud
Ansible
APIs
APIs?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Curious, is this a regex ? or a regular ??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regex, means that it accepts API and APIs

(?<!<)(?<=Canonical )Observability Stack: Observability Stack
(?<!<)Snapcraft(?!>|\.io): Snapcraft
(?<!<)snapd(?!>): snapd # Vale seems to allow normal capitalisation, may need specific existence rule for Snapd
(?<!<)Ubuntu Core: Ubuntu Core
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the logic for which names need (?!>) after? Should this and the observability stack entries have it too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So this allows for ref links and such, which don't support spaces - which I think means I didn't need to apply it to the start of Observability Stack - or some of the others.

Comment thread tests/data/manifest.yml
Comment on lines +235 to +238
Lets try websites:
snapcraft.io is the Snapcraft website.
api.snapcraft.io is the Snapcraft API site.
https://api.snapcraft.io is using a protocol.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add a couple of usages with angle brackets, to check that they're not flagged? To exercise the rules and also as representative of real docs (I'm curious how the angle brackets appear in practice).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, a good idea. This is the trouble with using a test file and then forgetting to translate some things to the formal test cases.

Comment thread tests/data/manifest.yml
The value is 999 items.
expect:
triggers: []
severity: suggestion
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about adding one more testcase:

Suggested change
severity: suggestion
- id: valid-large-number-in-code
filetypes: [md]
content: |
The machine ID is `123345`.
expect:
triggers: []

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For negative cases we can just add to the content of existing negative cases - and will do :)

level: warning
tokens:
- '^[ \t]*[$%]'
- '^[ \t]*[$%] \w+'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This won't catch things like:

$ ./run-me.sh
$ . .venv/bin/activate

Maybe that's OK in practice. If we want to cover these cases, how about:

Suggested change
- '^[ \t]*[$%] \w+'
- '^[ \t]*[$%] [-\w./]+'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A good point, I think it could just be:

Suggested change
- '^[ \t]*[$%] \w+'
- '^[ \t]*[$%] .+'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
- '^[ \t]*[$%] \w+'
- '^[ \t]*[$%] [^\s]+'

Works better it seems.

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