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

Adding topic detail model #39

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dannykopping
Copy link
Contributor

Adding topic detail model to express additional topic detail requirements in the fetchAndLock call

@dannykopping
Copy link
Contributor Author

@amalagaura more just submitting this for your comment - is there a better way we could do this?
In my use-case, we need to be able to send the processDefinitionVersionTag param as part of the fetchAndLock call

@dannykopping dannykopping force-pushed the dynamic-topic-properties branch from 77b04ce to 71c852e Compare January 22, 2020 09:38
@amalagaura
Copy link
Owner

@dannykopping This seems ok, let me take a look at this and see what else I can find. Give me a day or two.

@amalagaura
Copy link
Owner

@dannykopping Any thoughts on using Her's nested attributes? I think it would mean creating a Her model for TopicDetail, which would be unusable. Then we would add a has_many and accepts_nested_attributes_for and then alias topics topics_attributes. But you get the automatic Ruby class for TopicDetail.

@amalagaura
Copy link
Owner

OK I see you don't care about the deserialization as much. It is only used for fetch_and_lock. This make sense then. It is just a PORO instead of passing in a hash I see.

@dannykopping
Copy link
Contributor Author

OK I see you don't care about the deserialization as much. It is only used for fetch_and_lock. This make sense then. It is just a PORO instead of passing in a hash I see.

Correct, ya. It's purely a request concern
As we use the API more and build out more support, it might be a nice idea to create a "request model" concept in the lib which all of these can be based off of

@amalagaura
Copy link
Owner

@dannykopping Yes I think I added the fetching and locking as an after thought. It was just a bare minimum. I didn't have any complex requirements for that.

I think your PR will work. You can add tests and I can merge unless you want to add anything else.

@dannykopping
Copy link
Contributor Author

Sweet, thanks Ankur - we'll add the tests tomorrow hopefully

@dannykopping dannykopping force-pushed the dynamic-topic-properties branch from 1d0aa5c to 42c0191 Compare February 25, 2020 06:42
@amalagaura amalagaura marked this pull request as draft September 4, 2020 01:24
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