Skip to content

Added Un-escaped Ampersands Checker for #107#108

Merged
koppor merged 12 commits into
JabRef:mainfrom
AkshatJain9:main
Oct 27, 2022
Merged

Added Un-escaped Ampersands Checker for #107#108
koppor merged 12 commits into
JabRef:mainfrom
AkshatJain9:main

Conversation

@AkshatJain9

Copy link
Copy Markdown
Contributor

I am currently working on #8948 in the main JabRef repository. As a part of that issue, we need to check that all Ampersands stored in each csv is un-escaped. This PR contains a simple Python Script which checks each CSV for this condition, throwing a value error if we find an escaped ampersand. The script is configured to run each time there is a push to the main branch using GitHub actions, and integrates well with the existing format/lint checkers. I am seeking some feedback on whether or not this integrates well with the existing workflow, as well as general code conventions and quality. There are also some minor formatting changes as the existing GitHub Actions job was failing.

Thanks in advance!

@AkshatJain9 AkshatJain9 marked this pull request as ready for review October 17, 2022 10:40
@Siedlerchr

Copy link
Copy Markdown
Member

Thanks for the contribution! That already looks good to me

@Siedlerchr Siedlerchr requested a review from koppor October 17, 2022 20:50
@AkshatJain9

Copy link
Copy Markdown
Contributor Author

Before (if) this is merged, could I be put as the assignee of the relevant issue? I am contributing as a part of a University Assignment so it would help me a lot admin-wise to show my marker. Thanks!

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Script looks good, some minor comments on the action and on the CHANGELOG change.

Comment thread .github/workflows/tests.yml
Comment thread CHANGELOG.md Outdated
@AkshatJain9

Copy link
Copy Markdown
Contributor Author

How does that look? I've separated the jobs and fixed the CHANGELOG as required. The only small optimisation I can think of is to perhaps abstract the repeated Checkout source code into its own function using GitHub Composite Actions, but at this stage it seems like it will bloat the file more than it will simplify it, since its such a small Actions workflow. Let me know what you think!

Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
@AkshatJain9

Copy link
Copy Markdown
Contributor Author

I hope I've fixed it now, let me know if any1 thing else is needed

@koppor

koppor commented Oct 27, 2022

Copy link
Copy Markdown
Member

I fixed the changelog issues for myself at 46856e1 (#108).

Thank you for working on this, I will merge.

@koppor koppor merged commit b4d0b84 into JabRef:main Oct 27, 2022
@AkshatJain9

Copy link
Copy Markdown
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants