Skip to content

Conversation

@lxy2304
Copy link
Collaborator

@lxy2304 lxy2304 commented Jul 7, 2024

No description provided.

@lxy2304 lxy2304 linked an issue Jul 7, 2024 that may be closed by this pull request
@lxy2304 lxy2304 requested a review from msonderegger July 7, 2024 03:56
@lxy2304 lxy2304 self-assigned this Jul 7, 2024
Copy link
Member

@mmcauliffe mmcauliffe left a comment

Choose a reason for hiding this comment

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

This looks great!

It might be worth looking into migrating information in setup.py to a either setup.cfg file like https://github.com/MontrealCorpusTools/Montreal-Forced-Aligner/blob/main/setup.cfg or into the pyproject.toml. At this point, I think the ideal is to have as little information in setup.py, though I don't think there's a way to do the windows-only dependency via configuration files.

@msonderegger
Copy link
Member

Hi @mmcauliffe @lxy2304 -- I trust Michael's review on this, so just let me know how to proceed. Does Xiaoyi address the things noted above, then I approve?

@lxy2304 lxy2304 linked an issue Jul 8, 2024 that may be closed by this pull request
@lxy2304
Copy link
Collaborator Author

lxy2304 commented Jul 8, 2024

@msonderegger Sounds good to me! I am working on the comments Michael suggested and will make a commit with the changes later.

@lxy2304 lxy2304 requested a review from mmcauliffe July 10, 2024 13:40
Copy link
Member

@msonderegger msonderegger left a comment

Choose a reason for hiding this comment

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

Looks great!

@msonderegger msonderegger merged commit 7f40cd4 into main Jul 11, 2024
@lxy2304 lxy2304 deleted the update-packaging branch July 11, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants