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 default class when original was not found #25

Conversation

bguban
Copy link
Contributor

@bguban bguban commented Dec 24, 2022

Fixes #24

@virtualstaticvoid
Copy link
Owner

Hi @bguban

Thanks for the PR!

May I request the following changes:

  1. It would be better to have the "replacement type" be called UnknownDefinition instead of the generic UninitializedConstant.
  2. The visit_type(attribute) is also for Job types, so it should handle the unknown job type scenario too. To keep it simple, the "replacement type" could be UnknownDefinitionOrJob instead of needing two types.
  3. Please add a test to cover the changes.

Thanks!

@bguban
Copy link
Contributor Author

bguban commented Jan 3, 2023

Hi @virtualstaticvoid

I can easily change the class name but I need help with tests. Any suggestions on where would be a proper place to check it? Should I create a separate test for RedisDeserializationVisitor or add an expectation to an existing test?

@virtualstaticvoid
Copy link
Owner

Good questions! I'm also wondering what the ramifications would be if retrying a task against the Unknown type will be, so will want to cover that in the tests too.

I'll have a think to see what makes sense.

Btw I gave your UI a try. Managed to get it running very easily as per the readme instructions. I guess this PR should get sorted so that it works properly if a module or job type is missing.

move TestJob et al into supporting role
add new supporting classes
NB: constrain sidekiq to versions < 7.x
@virtualstaticvoid virtualstaticvoid merged commit 373c27c into virtualstaticvoid:master Jan 7, 2023
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.

Taskinator::Api::Processes fails when one of the classes doesn't exist any more
2 participants