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

Feature/initial implementation #1

Closed
wants to merge 95 commits into from

Conversation

sitepark-veltrup
Copy link
Member

Initial implementation of the atoolo search.

Copy link
Member

@sitepark-schaeper sitepark-schaeper left a comment

Choose a reason for hiding this comment

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

looks good so far, just a couple small things remaining

@sitepark-veltrup
Copy link
Member Author

The following issues apply to most Commands:

  • There are a couple of phpcs:ignore comments scattered which I believe we don't need.
  • The resource-dir and path arguments beeing hard to differentiate.
  • The Commands that use a builder-pattern should check if all (required) properties have been set when build() is invoked.

obsolete

Copy link
Member

@sitepark-schaeper sitepark-schaeper left a comment

Choose a reason for hiding this comment

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

What about this comment from earlier on?:

For the big constructors (especially) it's probably usefull to use named arguments. Compare the following:

        $this->status = new IndexerStatus(
            IndexerStatusState::RUNNING,
            new DateTime(),
            null,
            $total,
            0,
            0,
            new DateTime(),
            0,
            0
        );
        $this->status = new IndexerStatus(
            state: IndexerStatusState::RUNNING,
            startTime: new DateTime(),
            endTime: null,
            total: $total,
            processed: 0,
            skipped: 0,
            lastUpdate: new DateTime(),
            updated: 0,
            errors: 0
        );

Copy link
Member

@sitepark-schaeper sitepark-schaeper left a comment

Choose a reason for hiding this comment

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

found a couple more instances where named arguments are probably worth using

src/Console/Command/MoreLikeThis.php Show resolved Hide resolved
src/Console/Command/SolrSelectBuilder.php Show resolved Hide resolved
src/Console/Command/SolrSelectBuilder.php Show resolved Hide resolved
src/Dto/Search/Query/SelectQueryBuilder.php Show resolved Hide resolved
src/Service/Search/SolrMoreLikeThis.php Show resolved Hide resolved
src/Service/Search/SolrSelect.php Show resolved Hide resolved
Copy link
Member

@sitepark-schaeper sitepark-schaeper left a comment

Choose a reason for hiding this comment

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

👍

@sitepark-veltrup
Copy link
Member Author

Included in #3

@sitepark-veltrup sitepark-veltrup deleted the feature/initial-implementation branch June 28, 2024 07:08
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