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

Fixes excessive line wrapping. #529

Merged
merged 8 commits into from
Apr 9, 2024
Merged

Conversation

kylebgorman
Copy link
Collaborator

@kylebgorman kylebgorman commented Apr 8, 2024

The Min Nan nan smoke test is broken. It seems that Min Nan has been removed from English Wiktionary, without a trace. So, I have proposed to delete its custom selector. @jacksonllee if you have a moment, could you make sure I'm not making things worse? (Is it perhaps now recorded as Hokkien or Fujian?)

  • Updated Unreleased in CHANGELOG.md to reflect the changes in code or data.

Copy link
Collaborator

@jacksonllee jacksonllee 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 overall. A small merge conflict for CHANGELOG.md needs to be resolved. I also have a question about "lxml_html_clean".

The Min Nan nan smoke test is broken. It seems that Min Nan has been removed from English Wiktionary, without a trace. So, I have proposed to delete its custom selector. @jacksonllee if you have a moment, could you make sure I'm not making things worse? (Is it perhaps now recorded as Hokkien or Fujian?)

Everything looks right to me with respect to dropping Min Nan here. Hokkien would be a good addition, but it currently has only 74 entries on Wiktionary. When it reaches our threshold of 100, we can pull in the data.

@@ -1,7 +1,8 @@
ipapy>=0.0.9.0
lxml_html_clean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were you trying to use lxml_html_clean somewhere? It's not used in this pull request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See note here:

21054c3

Tests fail without it, and including it is the suggested solution.

@kylebgorman kylebgorman merged commit 3538190 into CUNY-CL:master Apr 9, 2024
10 checks passed
@kylebgorman kylebgorman deleted the wrap branch April 9, 2024 13:13
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.

None yet

2 participants