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

Separate WikiNode.args into WikiNode.sarg and WikiNode.largs #87

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

kristian-clausal
Copy link
Collaborator

WikiNode previously had one attribute that held two kinds of argument data: either a single string, for things like HTML tag names, List "##*"-style level strings etc., or a list of lists containing more complicated data, like Template arguments and stuff.

It's really hard to keep track of, and a really annoying thing to write well-typed code for. Because by necessity you'd need to have it be a [str | List[List[...]]], you have to constantly create phantom asserts either with # type: ignore or if TYPE_CHECKING: assert... to make type-checkers happy.

sarg and largs are both preliminary names: string argument and list arguments, specifically, but if someone can come up with something better, these two should be unique enough to be easily replaceable.

WikiNode previously had one attribute that held two kinds of
argument data: either a single string, for things like HTML
tag names, List "##*"-style level strings etc., or a list
of lists containing more complicated data, like Template
arguments and stuff.

It's really hard to keep track of, and a really annoying thing to
write well-typed code for. Because by necessity you'd need to
have it be a [str | List[List[...]]], you have to constantly
create phantom asserts either with `# type: ignore` or `if
TYPE_CHECKING: assert...` to make type-checkers happy.

`sarg` and `largs` are both preliminary names: string argument
and list arguments, specifically, but if someone can come up
with something better, these two should be unique enough to be
easily replaceable.
@xxyzz
Copy link
Collaborator

xxyzz commented Aug 17, 2023

IMO the problem is the WikiNode class is too general, rename the attribute just make the type checking easier but not make writing extractor code easier. You mentioned another approach before: create class inherent the WikiNode class, and the type and attribute name could be changed accordingly. Like a HTML node could be HTMLNode and has a str type tag attribute. And template node could have a dict type arguments attribute, a str type template_name attribute. But I'm not sure the implantation complicity. I'm not against the change though we could improve it later.

@kristian-clausal
Copy link
Collaborator Author

It does feel like we could use a lot more classes than we do now, especially when using different WikiNode subclasses is almost free, because WikiNodes are already... classed objects, you know what I mean.

However, this is the smallest change I could think of that shouldn't break anything, and it will also make it easier to do later changes (.sarg and .largs are easier to look for than.args which already has overlap in a ton of Lua code). If there's no big issues, I think we can safely merge this change.

@kristian-clausal kristian-clausal merged commit d5d6265 into main Aug 17, 2023
4 checks passed
@kristian-clausal kristian-clausal deleted the wikinodeargs branch August 17, 2023 10:27
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.

2 participants