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

Support customizing the tag name #11

Closed
wants to merge 3 commits into from
Closed

Conversation

Fale
Copy link
Contributor

@Fale Fale commented Aug 16, 2018

No description provided.

@coveralls
Copy link

coveralls commented Aug 16, 2018

Coverage Status

Coverage increased (+0.2%) to 82.558% when pulling b829834 on Fale:custom_group_name into bb4d97c on liip:master.

@Fale
Copy link
Contributor Author

Fale commented Aug 16, 2018

This change is related to https://github.com/Fale/sheriff

This (Fale@921a76a) is more precisely what I look forward, but the only way I can think of to obtain it is to change the API, which might be something that would be better avoiding.

I'm looking for that functionality more than that syntax, so if we can figure out a way to implement that in a way that is satisfying for you to merge, I'll be very happy to reimplement it in such way!

@mweibel
Copy link
Collaborator

mweibel commented Aug 16, 2018

thanks for your PR. I'm not quite sure yet why you need that. Initial reaction was: why don't you just define more groups?
I'm quite tired though, so I'll re-read that tomorrow morning and see if I can make sense out of it ;)

@Fale
Copy link
Contributor Author

Fale commented Aug 16, 2018

Groups are managed in or (have at least in the group), while those are in and.

so basically is:

(X or Y or Z) AND (A or B or C) AND ...

Real case scenario (the one I'm using it for):

type Service struct {
    ID          uuid.UUID    `json:"id"             gorm:"primary_key"  type:"Read,List"    caps:"ServiceRead,SuperRead"`
    Code        string       `json:"code"                               type:"Read,List"    caps:"SuperCreate,ServiceRead,SuperRead"`
    Name        string       `json:"name"                               type:"Read,List"    caps:"SuperCreate,SuperRead"`
    Type        string       `json:"type"                               type:"Read,List"    caps:"SuperCreate,ServiceRead,SuperRead"`
    SubServices []SubService `json:"sub_services"                       type:"Read"         caps:"ServiceRead,SuperRead"`
}

we use caps (capabilities) to decide what user can do what, while type (request type) to allow us to have a single struct for both single objects GETs and lists GETs.
The reason why our lists have less properties is that we do not want to load related objects in lists, since those would be multiple queries, more data moved, and those would be wasted since our clients would still issue the per-object call

@mweibel
Copy link
Collaborator

mweibel commented Aug 28, 2018

Hi @Fale

I see the use case now but I'm unsure yet if there isn't a better way to integrate it with a smaller API change possibly.
Though this change is rather small, maybe there's an even simpler way. Just speculating here, not having had much thought into it yet.
Thing is, I'll soon leave for a longer vacation with no access to internet ;) Would you mind if I postpone this PR until I'm back?

Sorry!

@Fale
Copy link
Contributor Author

Fale commented Aug 28, 2018

Hi,

I'm not sure either if this is the best approach.
I'm fine keeping the PR on hold, since internally we are using https://github.com/Fale/sheriff which allows multiple groups as well and we have been on that codebase for quite a few months, and I have waited to opensource it since I'm not sure of the implementation, so few more weeks/months will be fine, even more so if by the end of the process sheriff obtains more features and the API is ultimately better :).

Cheers,
Have a nice vacation,
Fale

@mweibel
Copy link
Collaborator

mweibel commented Aug 28, 2018

Great! thanks. Let me know your results then ;)

@mweibel
Copy link
Collaborator

mweibel commented Mar 8, 2019

this PR now conflicts and I'm still waiting for an update in your case.
I'll keep it open for another few weeks, otherwise I'm gonna close it with no update from you.

@mweibel
Copy link
Collaborator

mweibel commented Jul 23, 2020

closing due to inactivity.

@mweibel mweibel closed this Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants