Skip to content

feat: escape Markdown syntax in changelog titles #697

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

Closed
wants to merge 6 commits into from

Conversation

germa89
Copy link
Contributor

@germa89 germa89 commented Feb 26, 2025

It seems there is not way to check if Markdown syntax is correct or not (Ref: https://stackoverflow.com/questions/25331366/markdown-syntax-checking-for-continous-integration)

Hence I am going with a more humble approach.

Close #696

@germa89 germa89 requested a review from a team as a code owner February 26, 2025 17:29
@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added the enhancement General improvements to existing features label Feb 26, 2025
@germa89
Copy link
Contributor Author

germa89 commented Feb 26, 2025

By the way, I keep having this, @klmcadams you might want to check.. maybe it is my python version though...

> git -c user.useConfigOnly=true commit --quiet --allow-empty-message --file -
black....................................................................Passed
isort....................................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

python-utils/parse_pr_title.py:247:46: E231 missing whitespace after ':'
python-utils/parse_pr_title.py:248:50: E231 missing whitespace after ':'
python-utils/parse_pr_title.py:251:45: E231 missing whitespace after ':'
python-utils/parse_pr_title.py:252:46: E231 missing whitespace after ':'

check for merge conflicts................................................Passed
check yaml...........................................(no files to check)Skipped
fix requirements.txt.................................(no files to check)Skipped
trim trailing whitespace.................................................Passed
debug statements (python)................................................Passed
Validate GitHub Workflows............................(no files to check)Skipped
Add License Headers..................................(no files to check)Skipped

@germa89 germa89 requested a review from klmcadams February 26, 2025 17:41
@germa89 germa89 self-assigned this Feb 26, 2025
@germa89
Copy link
Contributor Author

germa89 commented Feb 26, 2025

Kinda shocking we do not have the changelog action activated on the changelog repo hahhaha

@RobPasMue
Copy link
Member

Kinda shocking we do not have the changelog action activated on the changelog repo hahhaha

#690 only for you... 🙃

@SMoraisAnsys
Copy link
Contributor

Kinda shocking we do not have the changelog action activated on the changelog repo hahhaha

We dont want to be our own nightmare :D

@SMoraisAnsys
Copy link
Contributor

I know we already discussed about it in the past but we might want to extend the scope of check-pr-title by checking that a title only contains alpha / digit / spaces characters ? From this changes requirement, I'm expecting other people coming in the future because they are using other characters leveraged in MD, e.g. [, ], >, #, ...
This would also adress #696 as people would we notified that they are using invalid characters.

@jorgepiloto jorgepiloto added this to the ansys/[email protected] milestone Feb 27, 2025
@germa89
Copy link
Contributor Author

germa89 commented Feb 27, 2025

Kinda shocking we do not have the changelog action activated on the changelog repo hahhaha

#690 only for you... 🙃

You saying I'm special?..... mmmhhh..... special, I like that...

https://youtu.be/aI0euMFAWF8?si=92b0RduZDJ5ugyG0&t=56

@jorgepiloto jorgepiloto changed the title feat: replacing * with \* to avoid parsing issues on doc builds feat: escape Markdown syntax in changelog titles Mar 3, 2025
@jorgepiloto
Copy link
Member

I know we already discussed about it in the past but we might want to extend the scope of check-pr-title by checking that a title only contains alpha / digit / spaces characters ? From this changes requirement, I'm expecting other people coming in the future because they are using other characters leveraged in MD, e.g. [, ], >, #, ... This would also adress #696 as people would we notified that they are using invalid characters.

I fully agree about this. Also, note that the changelog templates using Sphinx-design cards may suffer from rendering issues if symbols are included.

Following @SMoraisAnsys suggestion, I would prefer to block the use of symbols rather than trying to escape them. This just adds more symbols to the fragment.

@RobPasMue
Copy link
Member

So, shall we just close this and tackle a refactoring that does not support any kind of rich syntax? (neither MD, rST...)

@SMoraisAnsys
Copy link
Contributor

SMoraisAnsys commented Mar 5, 2025

So, shall we just close this and tackle a refactoring that does not support any kind of rich syntax? (neither MD, rST...)

I agree with closing this PR. Note that it would be wise that the refactoring is compatible with !, ( and ) since we emphasize the use of conventional commit and this are symbols used to define scope and breaking changes. See #722 for example :D

@jorgepiloto
Copy link
Member

Closing this after the discussion.

@jorgepiloto jorgepiloto closed this Mar 6, 2025
@germa89 germa89 deleted the feat/checking-PR-markdown-syntax branch March 12, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add syntax check on the changelog action
5 participants