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

Remove some code in the preview section that isn't needed #402

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Mar 3, 2023

Description of the Change

In #351, a previewer was added to the admin. This makes a request to the Classifier::get_body method, passing in some options. The problem here is that this method expects the options array to look like this:

[
    'text' => 'Some Text'
    'language' => 'en',
    'features' => [
        'categories' => [],
        'concepts' => [],
        // etc
    ],
]

But all we are passing in is a single array containing features part, which looks like:

[
    'categories' => [],
    'concepts' => [],
    // etc
]

These values end up being ignored because they don't follow the format we expect. In addition, these values are the same as the default values in the Classifier::get_body method, so there's no real reason I can see to pass those in the first place. I discovered this while using the preview section to quickly verify a new feature and found out the values I was passing in weren't being recognized.

This PR removes those and also changes the default limit value to match how we set the limit in other places (using our constant first). An alternative approach here is to change the format of the data we pass in to match what we expect but because we just pass in default values, doesn't seem to add any value currently.

How to test the Change

Ensure the preview area still functions as expected

Changelog Entry

Fixed - Removed some unnecessary code in the preview feature

Credits

Props @dkotter

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@dkotter dkotter requested a review from a team as a code owner March 3, 2023 20:35
@dkotter dkotter self-assigned this Mar 3, 2023
@dkotter dkotter requested a review from jeffpaul as a code owner March 3, 2023 20:35
@dkotter dkotter requested review from Sidsector9 and removed request for jeffpaul and a team March 3, 2023 20:36
@jeffpaul jeffpaul added this to the 1.9.0 milestone Mar 3, 2023
@jeffpaul
Copy link
Member

jeffpaul commented Mar 3, 2023

@dkotter as long as the end result is still "user selects a sample post and can preview the results that Watson returns based on the enabled features & thresholds" then that's fine by me

@dkotter
Copy link
Collaborator Author

dkotter commented Mar 8, 2023

@jeffpaul Yeah, this doesn't change any functionality, everything should still work exactly the same. Just removes code that wasn't actually being used because it was in the wrong format. Could fix the format so it is used but since it's exactly the same as the default data, seemed better to just remove it.

@jeffpaul
Copy link
Member

jeffpaul commented Mar 9, 2023

@Sidsector9 would be great to wrap up review here so this can get merged before end of month when I'd like to get out the next major release

Copy link
Member

@Sidsector9 Sidsector9 left a comment

Choose a reason for hiding this comment

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

Approving as the tests are failing not because of the changes in this PR.

@dkotter dkotter merged commit f890b4d into develop Mar 13, 2023
@dkotter dkotter deleted the fix/preview-features branch March 13, 2023 15:19
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.

3 participants