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

Add getSegmentsByClass #122

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

jay-knight
Copy link
Contributor

This introduces a function that allows you to call:

$message->getSegmentsByClass(PID::class);

instead of

$message->getSegmentsByName("PID");

just to give a little more safety. I wanted to keep this very simple to see if this approach was seen as useful. But I could see functions like

function getFirstPIDSegmentInstance(): PID

That specifies a specific segment type as the return type, so that you could then call

$pid = $message->getFirstPIDSegmentInstance();
if (!is_null($pid)) {
    $race = $pid->getRace();
}

Because currently static analyzers like phpstan will complain that Segment doesn't have a function getRace().

Let me know if this you think this is useful and/or a good direction, and I can update docs.

@senaranya
Copy link
Owner

This is definitely useful. Thank you for the PR. Please go ahead and update the docs

* @param string $name Segment class
* @return array List of segments identified by class
*/
public function getSegmentsByClass(string $class): array
Copy link
Owner

Choose a reason for hiding this comment

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

Rename $class to $segmentClass

{
$segmentsByClass = [];

foreach ($this->segments as $seg) {
Copy link
Owner

Choose a reason for hiding this comment

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

Rename $seg to the full word $segment

@jay-knight
Copy link
Contributor Author

Thanks, @senaranya. I've now added:

  • Message::getSegmentsByClass()
  • Message::removeSegmentsByClass()
  • MessasgeHelpersTrait::hasSegmentOfClass()
  • MessasgeHelpersTrait::getFirstSegmentInstanceByClass()

I've made the suggested changes and ran phpdoc-md.

@senaranya
Copy link
Owner

Thanks for the changes, although it looks like the documentation changes are too big (probably because those weren't updated in the earlier PRs), and thus the actual diff for this is kind of lost in the noise. Let's revert those documentation changes please, unless you've added anything in the Readme file. We'll run phpdoc-md in a separate ticket. Also check if the Action (CI pipeline) failures are anything relevant

@jay-knight jay-knight force-pushed the get-segments-by-classname branch 2 times, most recently from b6aa463 to 2f81885 Compare September 29, 2024 19:19
@jay-knight jay-knight force-pushed the get-segments-by-classname branch from 2f81885 to c373704 Compare September 29, 2024 19:19
@jay-knight
Copy link
Contributor Author

@senaranya I've reverted the documentation update and I've fixed the ci errors.

@senaranya senaranya merged commit d038d96 into senaranya:master Oct 15, 2024
3 checks passed
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