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

Adjust Vale rules to ignore links #753

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Tokesh
Copy link
Collaborator

@Tokesh Tokesh commented Dec 22, 2024

Description

Updated the process_file method to remove markdown links, leaving only their text content. This ensures the tool processes description and x-deprecation-message fields cleanly, aligning with Vale style-checking requirements.

Issues Resolved

[#752]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

github-actions bot commented Dec 22, 2024

Changes Analysis

Commit SHA: 03cbf9d
Comparing To SHA: a2afe56

API Changes

Summary

NO CHANGES

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/12485091459/artifacts/2359970619

API Coverage

Before After Δ
Covered (%) 606 (59.35 %) 606 (59.35 %) 0 (0 %)
Uncovered (%) 415 (40.65 %) 415 (40.65 %) 0 (0 %)
Unknown 43 43 0

@Tokesh Tokesh added the skip-changelog No need to update CHANGELOG. label Dec 22, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

  1. Add tests, make sure there's one with 1 link and 2 links to https://github.com/opensearch-project/opensearch-api-specification/tree/main/tools/tests/prepare-for-vale.
  2. Extract line.replace(/\[([^\]]+)\]\([^)]+\)/g, '$1') into a method, similar to prune_vars, maybe something descriptive like remove_links.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

One more place where the transformation needs to be applied, see below.

Iterate till unit tests and linter are green (this repo uses a slightly different convention that the default TS with variable_names).

Add an integration test to https://github.com/opensearch-project/opensearch-api-specification/blob/main/tools/tests/prepare-for-vale/fixtures.

@@ -45,7 +45,7 @@ export default class KeepDescriptions {
} else if (inside_text && line.match(/^[\s]*[\w\\$]*:/)) {
inside_text = false
} else if (inside_text) {
const cleaned_line = line.replace(/\[([^\]]+)\]\([^)]+\)/g, '$1')
const cleaned_line = this.remove_links(line)
Copy link
Member

Choose a reason for hiding this comment

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

You can also have links in descriptions on line 44, so it will need to be handled there as well (and possibly in other cases). Add tests for those of course.

We have remove_links, prune_vars and occasionally prune. Either extract the common parts into a method, or at least make it visible/more readable that we're doing a series of transformations.

var cleaned_line = this.prune(line, /(description|x-deprecation-message):/, ' '))
cleaned_line = this.remove_links(line)
cleaned_line = this.prune_vars(cleaned_line)
fs.writeSync(writer, cleaned_line)
let cleaned_line = this.remove_links(line)
cleaned_line = this.prune_vars(cleaned_line)
fs.writeSync(writer, cleaned_line)

@Tokesh Tokesh requested a review from dblock December 23, 2024 17:25
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

The way this works is that we remove all text that doesn’t need to be checked.

So, you have to replace the text with an equal number of spaces otherwise the comments from the tool appear in the wrong place on GitHub.

@Tokesh Tokesh requested a review from dblock December 24, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No need to update CHANGELOG.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants