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

Add function to check python readme links are valid and absolute #739

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

Conversation

brokoli777
Copy link

@brokoli777 brokoli777 commented Oct 2, 2024

I added a check for the python README file to check if all the links are valid and not relative.

If the links are not valid, it will print out the invalid links for review and stops the script (stopping the package build in the process).

Currently, this is assuming that all the links in the README file will be pointing to GitHub, if that is not the case, I can modify it to check if it is an HTTPS link.

Closes #730.

Copy link

google-cla bot commented Oct 2, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@reyammer reyammer changed the title Add function to check python readme links are valid and absolute #730 Add function to check python readme links are valid and absolute Oct 4, 2024
Copy link
Collaborator

@reyammer reyammer left a comment

Choose a reason for hiding this comment

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

Please do pass with ruff on this script (see https://github.com/google/magika/actions/runs/11150217299/job/31080639258?pr=739)

I suggest to setup ruff to be run "on save" in your IDE (eg vscode).

python/scripts/build_python_package.py Outdated Show resolved Hide resolved
python/scripts/build_python_package.py Outdated Show resolved Hide resolved
python/scripts/build_python_package.py Outdated Show resolved Hide resolved
python/scripts/build_python_package.py Show resolved Hide resolved
python/scripts/build_python_package.py Outdated Show resolved Hide resolved
python/scripts/build_python_package.py Outdated Show resolved Hide resolved
@reyammer
Copy link
Collaborator

reyammer commented Oct 4, 2024

Thanks for the PR! Did a first review. Let me know if you have questions.

@reyammer
Copy link
Collaborator

Hello @brokoli777, still interested in helping out on this or should I take from here?

@brokoli777
Copy link
Author

Oh very sorry! For some reason, I didn't realize there was a reply to the pull request. I will do the recommended changes right now.

@reyammer
Copy link
Collaborator

Thanks, new round of comments:

  • ruff pass still fails, can you double check?
  • in case of errors, it seems the script just prints errors and does not really exit with 1?
  • I saw that the new function takes as input a markdown_path (which indicates it's a path to a given file), but the code tries to check whether it's a directory and does some recursive check to find new markdown. I find this confusing, I propose to keep it simple: the function should take a markdown_path (as it is now), and this should just be a path to a given markdown file; let's remove directory checks or anything like this. Unneeded complexity. So, let's move the code from the inner function to the outer function and then remove the inner function itself. and let's make sure it does exit with 1 in case of problems. Let's also update the comment to reflect that (focus on one markdown, not one or more).

Thanks!

@brokoli777
Copy link
Author

Hi, I believe I completed the mentioned suggestions. Let me know if it works on your end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GH workflow to check that the python package README does not contain relative paths
2 participants