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

Ensure README is synched with code #43

Merged
merged 12 commits into from
Oct 10, 2023

Conversation

jarrodmillman
Copy link
Member

@jarrodmillman jarrodmillman commented Oct 3, 2023

Following up on a suggestion by @bsipocz #30 (comment).

There is an issue with string literals in the toml file.

2023-10-02T21:15:36,110408044-07:00

If this approach seems reasonable, we may want to do the same thing for:

  • .github/workflows/label-check.yaml
  • .github/workflows/milestone-merged-prs.yaml

@jarrodmillman jarrodmillman added type: Documentation Improvements or additions to documentation type: Maintenance Refactoring and maintenance of internals labels Oct 3, 2023
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

I was thinking about how to solve this and was undecided whether to take the pre-commit hook approach (this one) or do it via a test suite which I've been meaning to work on next.

Happy to take this approach, as long as it doesn't grow to complex. It can be a bit vague to figure out why a commit just failed and what the pre-commit hooks just updated.

.tools/readme.py Show resolved Hide resolved
.tools/readme.py Outdated Show resolved Hide resolved
.tools/readme.py Show resolved Hide resolved
README.md Show resolved Hide resolved
@jarrodmillman
Copy link
Member Author

@lagru Do you have any suggestions for how we can resolve the regex issue with string literals in the toml file?

Replacement strings used in regex substitution process backlash escapes
including R"\\". So we need to expand these to R"\\\\" before [1].

[1] https://docs.python.org/3/library/re.html#re.sub
and pin paths relative to the scripts position. Should be a bit more
robust.
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

32f92b6 should address the "\\" issue. I also took the liberty to make the path handling a bit more robust and independent of the working directory 311faa8. Feel free to revert.

Co-authored-by: Lars Grüter <lagru+github@mailbox.org>
.tools/readme.py Outdated
with open(PROJECT_ROOT / "README.md") as fh:
readme = fh.read()

with open(PROJECT_ROOT / "src/changelist/default_config.toml") as fh:
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: does windows like this? Or we shouldn't be bothered anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean the "/" vs "\" thing? I think abstracting that away is one of the ideas behind pathlib but I haven't used in much on a Windows system to be honest.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Thanks @jarrodmillman!

.tools/readme.py Outdated Show resolved Hide resolved
.tools/readme.py Outdated Show resolved Hide resolved
@jarrodmillman jarrodmillman merged commit ac56307 into scientific-python:main Oct 10, 2023
2 checks passed
@jarrodmillman jarrodmillman added this to the 0.5 milestone Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Documentation Improvements or additions to documentation type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants