-
Notifications
You must be signed in to change notification settings - Fork 3
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
refactor: introduce namespace enum #806
base: main
Are you sure you want to change the base?
Conversation
enum MediawikiNamespaces { | ||
public const item = 120; | ||
public const property = 122; | ||
public const lexeme = 146; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity: why are these living on an enum
when they don't use enum
features? I.e. right now, this could also be a class
and nothing changes as far as I can tell. If it'd be an enum
as shown in the docs, it would probably use backed cases:
enum MediawikiNamespace {
case Item = 120;
case Property = 122;
case Lexeme = 146;
}
and \App\Traits\PageFetcher::fetchPagesInNamespace(string $wikiDomain, int $namespace): array
could become \App\Traits\PageFetcher::fetchPagesInNamespace(string $wikiDomain, MediawikiNamespace $namespace): array
, enforcing expected values at type level already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question; I actually switched from an interface
to an enum
last minute after thinking I was being clever to use a shiny new 8.1 feature but then did it fairly incompetently.
Thanks for the suggestion. Note it was also necessary to explicitly call ->value
on the new enum
in the PageFetcher
In order to reudce the chance of future mixups lets use an enum for mediawiki namespaces. Athough these namespace ids are not actually globally fixed within the wikibase cloud and wbstack context we force them to be set this way.
78fa0c9
to
60315ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: do we cut a release for changes like this because we can or do we wait for something else to ship it?
In case you choose the former, an entry in the changelog would be sweet.
@tarrow Any intention on still merging this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will be happy to see this merged
In order to reduce the chance of future mixups lets use an enum for mediawiki namespaces. Athough these namespace ids are not actually globally fixed within the wikibase cloud and wbstack context we force them to be set this way.