Refactor #5457: Prepend 'pulp' to fields on Model - Pulp

Actions

Send by e-mail Copy link

Refactor #5457

closed

Story #5443: [Epic] Remove leading _ from pulpcore model fields

Prepend 'pulp' to fields on Model

Added by dkliban@redhat.com about 4 years ago. Updated almost 4 years ago.

Status:

CLOSED - CURRENTRELEASE

Priority:

Normal

Assignee:

fao89

Category:

Sprint/Milestone:

3.0.0

Start date:

Due date:

% Done:

100%

Estimated time:

Platform Release:

Groomed:

Sprint Candidate:

Tags:

Sprint:

Sprint 61

Quarter:

Description

pulpcore.app.models.Model has three fields[0] that all start with an _. All these fields need to drop the _ prefix from their names.

_id will become pulp_id
_created will become pulp_created
_last_updated will become pulp_last_updated

The URL field name, _href, is defined in the settings.py[0].

_href will become pulp_href

[0] https://github.com/pulp/pulpcore/blob/3c62889e0c01fe54de66c60147192a03cac73ab2/pulpcore/app/models/base.py#L21-L23
[1] https://github.com/pulp/pulpcore/blob/3c62889e0c01fe54de66c60147192a03cac73ab2/pulpcore/app/settings.py#L117

Actions

Copy link

Updated by dkliban@redhat.com about 4 years ago

Tracker changed from Issue to Refactor
% Done set to 0

Actions

Copy link

Updated by dkliban@redhat.com about 4 years ago

Description updated (diff)

Actions

Copy link

Updated by dalley about 4 years ago

If we remove the leading _ from _id then we're back to the place we were originally -- we need to find a new name for the pk field, or else, plugins will need to avoid "id", which is a pretty common name for a field.

Either would work, but we should decide on a strategy going forwards.

e.g. UpdateRecord https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/models.py#L386

The same applies to _href -> href, if we decide to do that.

https://github.com/pulp/pulp_rpm/blob/master/pulp_rpm/app/models.py#L666

I have no example of "last_updated" but I feel like it would be a likely future conflict.

Personally, I propose using a "pulp_" prefix.

Actions

Copy link

Updated by dkliban@redhat.com about 4 years ago

Adding a 'pulp' or 'pulpcore' to the field names would eliminate the chance of conflict between fields provided by pulpcore and the plugin.

pulp_id
pulp_href
pulp_created
pulp_last_updated

The names are very descriptive. They eliminate ambiguity for the REST API users.

Actions

Copy link

Updated by bmbouter about 4 years ago

dkliban@redhat.com wrote:

Adding a 'pulp' or 'pulpcore' to the field names would eliminate the chance of conflict between fields provided by pulpcore and the plugin.

pulp_id
pulp_href
pulp_created
pulp_last_updated

The names are very descriptive. They eliminate ambiguity for the REST API users.

+1, these names are descriptive and they avoid the conflicts so I think that is the goal.

Actions

Copy link

Updated by dkliban@redhat.com about 4 years ago

Subject changed from Remove leading _ from fields on Model to Prepend 'pulp' to fields on Model
Description updated (diff)

Actions

Copy link

Updated by dkliban@redhat.com about 4 years ago

Actions

Copy link

Updated by daviddavis about 4 years ago

+0. Not a big fan of this change but I see no other path forward.

Actions

Copy link

Updated by ttereshc about 4 years ago

+0.75 for prepending everything with pulp_. I agree with @daviddavis.

Actions

Copy link

#10

Updated by fao89 about 4 years ago

Sprint set to Sprint 60

Actions

Copy link

#11

Updated by fao89 about 4 years ago

Status changed from NEW to ASSIGNED
Assignee set to fao89

Actions

Copy link

#12

Updated by dalley about 4 years ago

Should we also change the names of "_versions_href" and "_latest_version_href" on RepositoryVersion? We would not need a "pulp_" prefix, but we would potentially want to drop the "_" prefix. I do not know whether that is an issue for the bindings the same way that the others were (as I don't think we filter on those fields at all), but since we're getting rid of that pattern everywhere else, it might no longer fit ergonomically speaking. And, it is not particularly necessary as nobody subclasses RepositoryVersion.

Actions

Copy link

#13