-
-
Notifications
You must be signed in to change notification settings - Fork 0
Snowflake identifier #5
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
base: 1.x
Are you sure you want to change the base?
Conversation
@roxblnfk This is now passing after those couple of PR's were merged. Feedback welcomed |
src/SnowflakeDiscord.php
Outdated
private int $workerId = 0, | ||
private int $processId = 0, |
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.
Looks like these fields shouldn't be hardcoded in attributes.
We can declare it as null|int<0, max>
and use hardcoded values if they were specified.
Need to think how to provide this options from config (where ENVs can be used) to behavior implementation.
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.
As I was writing these classes this did cross my mind. For instance SnowflakeInstagram
and shardId
is something developers will want more control over, if using the identifiers to their full potential/purpose.
The same applies for UUID
's when specifying a node
or localDomain
.
I moved on pretty quick but the idea I had was trivial, a static class allowing developers to define values for these behaviors at a time which makes sense to them. E.g. application bootstrap, after passing configuration, environment variables etc. The behavior class constructors would be modified, accepting and defaulting to a null
value. When not specified getListenerArgs
would use a value from the <Identifier>Defaults
class.
Just an idea, open to others.
final class SnowflakeDiscordDefaults
{
private static int $workerId = 0;
private static int $processId = 0;
public static function getWorkerId(): int
{
return self::$workerId;
}
public static function setWorkerId(int $workerId): void
{
self::$workerId = $workerId;
}
public static function getProcessId(): int
{
return self::$processId;
}
public static function setProcessId(int $processId): void
{
self::$processId = $processId;
}
}
final class SnowflakeDiscord extends BaseSnowflake
{
public function __construct(
string $field = 'snowflake',
?string $column = null,
private ?int $workerId = null,
private ?int $processId = null,
bool $nullable = false,
) {
}
protected function getListenerArgs(): array
{
return [
'field' => $this->field,
'workerId' => $this->workerId === null ? SnowflakeDiscordDefaults::getWorkerId() : $this->workerId,
'processId' => $this->processId === null ? SnowflakeDiscordDefaults::getProcessId() : $this->processId,
'nullable' => $this->nullable,
];
}
}
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.
Implemented similar to above, however defaults are considered within __construct
instead of getListenerArgs
. Done for all relevant identifiers. @roxblnfk Please review/resolve when you get a moment keen to 🚢 and wrap up an initial implementation.
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.
Unsure why there's a delay in merging? If something additional or changes are needed, let me know.
🔍 What was changed
🔔 Note
Pipelines will fail until the following PR's are merged and released.
📝 Checklist
💯 Test coverage
🟢 Psaml with empty baseline