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

apply one-sentence-per-line rule for rst files via github workflow. #4996

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions .github/workflows/rst_check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
name: grammar / rule check for .rst files

on:
pull_request:
paths:
- "**/*.rst" # trigger only for .rst files

jobs:
one_sentence_per_line:
runs-on: ubuntu-latest

steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: "3.10"

- name: Get changed lines in .rst files
run: |
git fetch origin ${{ github.base_ref }} --depth=1
git diff --unified=0 origin/${{ github.base_ref }} HEAD -- '*.rst' \
| grep '^[+-]' | grep -Ev '^(---|\+\+\+)' > /tmp/changed_lines.txt || true

- name: Run one-sentence-per-line checker on changed lines
run: |
python3 ./scripts/sentence_line_checker.py /tmp/changed_lines.txt
66 changes: 66 additions & 0 deletions scripts/sentence_line_checker.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# Copyright 2025 Sony Group Corporation.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.


import argparse
import re
import sys

def is_one_sentence_per_line(line) -> bool:
"""
Check if a line contains only one complete sentence.

:param line: The line of file to check.
"""
# this assumes a sentence ends with a period, question mark, or exclamation mark.
sentence_endings = re.findall(r'[.!?]', line)
Comment on lines +26 to +27
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could be more rules applied here? i would like to have some feedback for more use cases.

Copy link
Member

@christophebedard christophebedard Feb 5, 2025

Choose a reason for hiding this comment

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

One way to find out would be to run it on all files and see.

Copy link

@Santti4go Santti4go Feb 12, 2025

Choose a reason for hiding this comment

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

Hmm one use case I can think of is for example using ellipses ....
As this is right now this f() would treat it as multiple sentences instead of just one.

Another not so strange use case could be using multiple exclamation marks: "This is one sentence!!!"

I suggest slightly changing the regex to allow multiple appearances. Well, at least that would solve the problems above.

Suggested change
# this assumes a sentence ends with a period, question mark, or exclamation mark.
sentence_endings = re.findall(r'[.!?]', line)
# this assumes a sentence ends with a period, question mark, or exclamation mark.
sentence_endings = re.findall(r'[.!?]+', line)

An small example applying above change
Notice that the last 3 sentences do not cause the test to fail, as expected.

image

# allow empty lines or lines with only one sentence
return len(sentence_endings) <= 1

def check_changed_lines(filename) -> None:
"""
Check only changed lines for violations.

.. warning:: It checks only added / modified lines for the viaolation in the file,

Choose a reason for hiding this comment

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

Suggested change
.. warning:: It checks only added / modified lines for the viaolation in the file,
::warning:: It checks only added / modified lines for the violation in the file,

and ignores everything else including removed lines.

:param filename: The name of the file to check.
"""
with open(filename, 'r', encoding='utf-8') as file:
lines = file.readlines()

violations = []
for lineno, line in enumerate(lines, start=1):
line = line.strip()
# check only added lines, ignore everything else
if line.startswith("+") and not is_one_sentence_per_line(line[1:]):
violations.append((lineno, line[1:]))

if violations:
print(f"\n⚠️ Found {len(violations)} violations: One sentence per line is required.")
for lineno, line in violations:
print(f" ❌ Line {lineno}: {line}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm, this is not user-friendly output, it would be better to output original file name and line to be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

see my comment about annotations: #4996 (comment)

# exit with non-zero status code to fail github actions
sys.exit(1)
else:
print("✅ No violations found.")

if __name__ == "__main__":
parser = argparse.ArgumentParser(
description="Check if modified contents contain only one complete sentence per line.")
parser.add_argument(
'filename', type=str, help='The name of the file to check.')
args = parser.parse_args()

check_changed_lines(args.filename)