This change makes sense to me but maybe the original behaviour is preferred for some reason.

I stumbled upon this when fetching all the labels for a number of groups and projects. I put them in a set() and ended up with a lot less items than I expected.

This change makes a lot of sense, thanks @ptalbert. For context, we didn't use to have _repr_attr so this may have some historical reason but I'm not entirely sure at the moment.

I think this might be a breaking change though, as people might be comparing labels for uniqueness, so I also wonder if we should be using iid rather than id, assuming it actually exists for labels (will have to check).

https://docs.gitlab.com/ee/api/labels.html only shows an id field, no iid.

I see the functional tests for groups and projects broke. I don't have docker so I cannot immediately run those tests locally. I will try to get something working.

Quite a few objects use name as their _id_attr :/

And there is a similar issue with ProjectMergeRequest objects. The _id_attr is iid. This means if you compare two MRs from completely different projects they will be seen as equal if they happen to have the same IID value.

Looks like epics and issues also suffer:

I imagine most of these objects never get compared but you never know, and certainly a Merge Request might. It seems prudent to fix these.

@ptalbert thanks. In some cases name is intentional, for example when there is no id returned (like for branches). So if there are other cases where this should not be the case, they would need to be analyzed individually.

The iid vs id is a known issue, one solution would be to change the eq/hash dunder methods (#935).

@ptalbert I think for others it mostly makes sense to keep name as _id_attr (e.g. see protected environments => https://docs.gitlab.com/ee/api/protected_environments.html#get-a-single-protected-environment).

But for this one it makes sense probably, we just need to make sure the tests pass and add a breaking change trailer in your commit message so we ensure a major release for this. Let me know if you need any help.

Hey @ptalbert, just wondering if you were still willing to work on this as it needs a little tweak it seems. Otherwise we can maybe pick it up in #2926.

This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days.