Skip to content

fix: re-cookie with scikit-package v0.1.0 #165

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 1 commit into from

Conversation

ycexiao
Copy link
Collaborator

@ycexiao ycexiao commented Feb 11, 2025

close #159

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.85%. Comparing base (6526430) to head (a3facde).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #165   +/-   ##
=======================================
  Coverage   96.85%   96.85%           
=======================================
  Files          18       18           
  Lines         795      795           
=======================================
  Hits          770      770           
  Misses         25       25           
Files with missing lines Coverage Δ
tests/test_morphchain.py 96.96% <ø> (ø)
tests/test_morphpdftordf.py 96.00% <ø> (ø)
tests/test_morphrdftopdf.py 96.00% <ø> (ø)
tests/test_morphresolution.py 95.83% <ø> (ø)
tests/test_morphrgrid.py 98.33% <ø> (ø)
tests/test_morphscale.py 95.65% <ø> (ø)
tests/test_morphshape.py 95.83% <ø> (ø)
tests/test_morphshift.py 96.15% <ø> (ø)
tests/test_morphsmear.py 96.42% <ø> (ø)
tests/test_morphstretch.py 97.22% <ø> (ø)
... and 3 more

@ycexiao ycexiao marked this pull request as ready for review February 11, 2025 01:06
@ycexiao
Copy link
Collaborator Author

ycexiao commented Feb 11, 2025

@bobleesj @sbillinge It's ready for review, thank you!

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Thanks @ycexiao please see inline comments

@Sparks29032 @bobleesj @Tieqiong please can you take a quick look at this? Like PDFgui, this has a manual that uses texinfo. This may need different handling in general than standard docs as we have been discussing with @Tieqiong. I think there are a few decisions ot make:

  1. migrate away from texinfo so we don't have to deal with this any more in the future in all projects
  2. as 1 but just for pdfmorph, whose manual may not be so extensive atm
  3. stay with texinfo and make a new workflow that handles this in all our projects.
  4. something else

@ycexiao please can you organize a meeting with everyone listed and let's each of us bring a 5 minute pitch of which option we advocate for, then we can discuss to consensus....

;; Frobenius norm used in np.linalg.norm
fro

;; library used for Python package release, no longer used
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more a question for @bobleesj but why is this here? Presumably we want to remove all references to rever if we are not using it any more, so having codespell flag it is helpful?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ycexiao Could you try to remove the rever keyword and see if pre-commit still passes?

Copy link
Contributor

Choose a reason for hiding this comment

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

the point of the linting is to make the code better, so we want it to fail when that allows us to make the code better. Just a general comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed rever and codespell raise an error

codespell................................................................Failed
- hook id: codespell
- exit code: 65

CHANGELOG.rst:70: rever ==> revert, refer, fever

In the CHANGELOG.txt 70, there is

 v0.0.1
====================

**Changed:**

 * Fixed rever GH address

I think adding the words ignore here is necessary. It lets .codespell check other contents in the CHANGELOG.txt.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, sounds good.

recommended that you consult online resources and become somewhat
familiar before using PDFmorph.

PDFmorph can be run with Python 3.10 or higher. It makes use of several third party
Copy link
Contributor

Choose a reason for hiding this comment

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

@ycexiao please can you make an issue to have @Sparks29032 revisit this readme and bring it up to date and remove unwanted stuff

@bobleesj
Copy link
Contributor

bobleesj commented Feb 11, 2025

@ycexiao Overall good work considering this is your first time running skpkg fully.

It appears that the doc paths have been modified - have you tried rendering the doc locally to ensure the paths are all correct?

@ycexiao
Copy link
Collaborator Author

ycexiao commented Feb 11, 2025

@ycexiao Overall good work considering this is your first time running skpkg fully.

It appears that the doc paths have been modified - have you tried rendering the doc locally to ensure the paths are all correct?

Sorry, but I want to ask what do we use to render docs?

@bobleesj
Copy link
Contributor

@ycexiao
Copy link
Collaborator Author

ycexiao commented Feb 11, 2025

@ycexiao please check the skpkg doc. https://billingegroup.github.io/scikit-package/scikit-package-guide.html#appendix-2-how-to-build-documentation-locally

I see. I rendered the documents locally and it doesn't perform well. I will fix this later.

@sbillinge
Copy link
Contributor

@ycexiao please check the skpkg doc. https://billingegroup.github.io/scikit-package/scikit-package-guide.html#appendix-2-how-to-build-documentation-locally

I see. I rendered the documents locally and it doesn't perform well. I will fix this later.

do you mean you will make an issue and fix it on a separate PR? Or will you close this PR and make a new PR without the path changes in the docs?

Please see my comment about working with @Tieqiong on the docs. We will probably move away from using texinfo for docs and so all those changes should be on a separate PR. To avoid a large diff could we redo this PR without those changes? Sorry about that.

@sbillinge sbillinge closed this Feb 12, 2025
@ycexiao
Copy link
Collaborator Author

ycexiao commented Feb 12, 2025

@ycexiao please check the skpkg doc. https://billingegroup.github.io/scikit-package/scikit-package-guide.html#appendix-2-how-to-build-documentation-locally

I see. I rendered the documents locally and it doesn't perform well. I will fix this later.

do you mean you will make an issue and fix it on a separate PR? Or will you close this PR and make a new PR without the path changes in the docs?

Please see my comment about working with @Tieqiong on the docs. We will probably move away from using texinfo for docs and so all those changes should be on a separate PR. To avoid a large diff could we redo this PR without those changes? Sorry about that.

These path changes are not related to texinfo here. But sure current PR is a bit messy here, I will redo the PR with cleaner commit history.

@ycexiao ycexiao deleted the package-release branch March 14, 2025 16:23
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.

chore: re-cookiecut with latest cookiecutter
3 participants