-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add "POST ../access" & "PUT ../{access_id}" #32
base: dev
Are you sure you want to change the base?
Conversation
Hey @uniqueg. Tried something different in this code (not similar to POST and PUT in object registration). Please do have a look. The main reason for this was that I was not able to cover some corner cases with other approaches that I had in mind. If you have any other ideas for implementation, please do suggest. TIA :) |
Thanks @sarthakgupta072. Should be able to look at this soon. But before I start: would you mind explaining in a bit more detail what you mean by "Tried something different" and "some corner cases"? Please also have a look at TRS-Filer where a very similar thing is already taken care of (objects & access methods vs tools & versions). Perhaps this will help. Not saying though that this is how it has to work. If you feel that your solution does the job and is elegant enough, I'll be happy to review that. |
Sure Alex. Will have a look and update you soon. ;) |
Hey @uniqueg. I'll just compare both Consider the case, where the Now, when I analyze both the approaches, I feel the approach in So, I'll make changes according to |
Sounds great, thanks a lot! |
Hey @uniqueg. Implemented this just like I'll add tests if everything looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sarthakgupta072 and sorry for the looong wait. It looks fine to me at first glance, but I didn't give an in-depth review. I'd say that if manual functional/integration tests pass (i.e., check that you can add/edit access methods and the controllers behave as expected), I think you can go ahead with writing unit tests.
Added two minor comments, a typo and replacing a function with an import from FOCA (less to unit test)
put: | ||
summary: Create or update an access method. | ||
description: |- | ||
Create a access method with a predefined ID. Overwrites any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create an access method
return data['access_id'] | ||
|
||
|
||
def generate_id( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can import that function from FOCA now, it should be available in 0.6.0
Description
Fixes #26 and #25
Type of change
Please delete options that are not relevant.
Checklist: