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

pronouns: fetch list from the pronoun.is GitHub repo at start #2130

Merged
merged 3 commits into from
May 10, 2022

Conversation

dgw
Copy link
Member

@dgw dgw commented Jun 30, 2021

Description

We don't have to wait until the 12th of Never for pronoun.is to have an API. Parsing tab-separated values is easy, and the file is already sorted according to approximate frequency for us according to their readme.

Includes a kill-switch config setting in case the repo ever gets deleted or reorganized, or the file format changes.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

Notes

Speaking of something that's over-complicated for what it does (as Exi said on IRC yesterday)… this also finds the shortest unique set of pronouns for each option to use in links. The only outlier cases now are they/them/their/theirs/themsel(ves|f), which won't be abbreviated as they/.../themsel(ves|f). I might or might not find the motivation to fix that.

@dgw dgw added this to the 8.0.0 milestone Jun 30, 2021
@dgw dgw requested a review from a team June 30, 2021 21:44
@dgw dgw force-pushed the pronouns-fetch branch from 691ec30 to 2faba88 Compare June 30, 2021 21:46
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

I don't really get the tree aspect of the pronoun parsing. I feel like I'm missing the problem that required a solution.

I also tried to run the code on the current pronoun file, and the result is quite different from the default set of pronouns. For example, instead of 'they/.../themselves', the parser returns 'they/them/their/theirs/themselves'. So, is it compatible? Why use ... in the default value, knowing that the actual file don't contain them? Especially given that the file will replace the default value, so if there is a feature here, it'll be made inaccessible.

A lot of confusion!

sopel/modules/pronouns.py Outdated Show resolved Hide resolved
sopel/modules/pronouns.py Outdated Show resolved Hide resolved
sopel/modules/pronouns.py Show resolved Hide resolved
sopel/modules/pronouns.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented Feb 24, 2022

I don't really get the tree aspect of the pronoun parsing. I feel like I'm missing the problem that required a solution.

Using a trie was to make it easier to match on prefixes. I think. It's been a few months. 😅

I've tried (😏) to address most of the style comments in fixup commits. Haven't gone through another round of testing just yet.

@dgw dgw requested a review from Exirel February 24, 2022 03:02
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

I still think this is a lot of work that I don't know how to justify. On the other hand, I didn't have to do anything, and it all looks good. Maybe I'm just annoyed there isn't any test for that yet, but there wasn't before anyway.

Let's move on.

dgw added 3 commits May 10, 2022 12:47
We don't have to wait until the 12th of Never for them to launch an API.
Parsing tab-separated values is easy, and the file is already sorted
according to approximate frequency for us according to their readme.
@dgw dgw force-pushed the pronouns-fetch branch from b6d708b to 14a7d4d Compare May 10, 2022 17:48
@dgw
Copy link
Member Author

dgw commented May 10, 2022

I have cleaned the history (squashing fixups) and fixed one last bug with the disambiguation output (in case the user specifies e.g. .setpronouns they, which matches multiple sets).

Testing also determined that it's not necessary to merge the fetched pronouns with the default set; they/.../themself (for example) works just fine without that entry existing in the dictionary. I dropped that change.

@dgw dgw merged commit beaa544 into master May 10, 2022
@dgw dgw deleted the pronouns-fetch branch May 10, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants