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

Fixes some HarvestCommand logging #4269

Merged
merged 5 commits into from
Sep 5, 2024
Merged

Fixes some HarvestCommand logging #4269

merged 5 commits into from
Sep 5, 2024

Conversation

paul-m
Copy link
Contributor

@paul-m paul-m commented Sep 1, 2024

Fixes [issue#]

Describe your changes

  • Removes logger as an injected dependency from HarvestCommands.
  • Converts all commands in HarvestCommands to use $this->logger() instead of alternatives.
  • Converts all message strings to use concatenation rather than interpolation.
  • Adds minimal automated test of commands.

QA Steps

Use sample_content module to create a harvest.

Try out the commands.

Checklist before requesting review

If any of these are left unchecked, please provide an explanation

  • I have updated or added tests to cover my code
  • I have updated or added documentation

@paul-m paul-m marked this pull request as ready for review September 4, 2024 20:31
Copy link
Contributor

@dmundra dmundra left a comment

Choose a reason for hiding this comment

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

Changes look solid to me and the commands work in my sandbox. @paul-m anything to be done about code climate https://codeclimate.com/github/GetDKAN/dkan/pull/4269 issue about shared code between archive and publish?

Copy link
Contributor

@dmundra dmundra left a comment

Choose a reason for hiding this comment

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

Code climate changes look good as well. Approved and merging.

@dmundra dmundra merged commit ee6d352 into 2.x Sep 5, 2024
11 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