-
Notifications
You must be signed in to change notification settings - Fork 671
feat(ux): display project.name_with_namespace on project repr #1996
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
Conversation
This change the repr from: $ gitlab.projects.get(id=some_id) <Project id:some_id> To: $ gitlab.projects.get(id=some_id) <Project id:some_id name_with_namespace:"group_name / project_name"> This is especially useful when working on random projects or listing of projects since users generally don't remember projects ids.
Cool. Just as an FYI we also have a
|
Codecov Report
@@ Coverage Diff @@
## main #1996 +/- ##
==========================================
+ Coverage 83.47% 92.51% +9.03%
==========================================
Files 78 78
Lines 4938 4943 +5
==========================================
+ Hits 4122 4573 +451
+ Misses 816 370 -446
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Overall looks good to me. Left a minor suggestion.
Unsure if we want a unit/functional test or not for this? @nejch what do you think?
|
||
if hasattr(self, "name_with_namespace"): | ||
return ( | ||
f'{project_repr[:-1]} name_with_namespace:"{self.name_with_namespace}">' |
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.
Can use the !r
to quote the value.
f'{project_repr[:-1]} name_with_namespace:"{self.name_with_namespace}">' | |
f"{project_repr[:-1]} name_with_namespace:{self.name_with_namespace!r}>" |
So don't tell @nejch 😜 But I still don't use |
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 @Psycojoker for this, I really like the idea to make repr more human-readable.
What do you think about generalizing it a bit to have a more declarative approach on individual resources (project, group,..) and handling the __repr__
based on that in the RESTObject
method, like we do with id
using _id_attr
now? Otherwise, we'll have to add a bunch more overrides if people want similar for other objects (or even GroupProject, UserProject.. etc).
For this, I'd introduce a _repr_attr
on individual resources, and handle that centrally here:
Lines 160 to 164 in 882fe7a
def __repr__(self) -> str: | |
if self._id_attr: | |
return f"<{self.__class__.__name__} {self._id_attr}:{self.get_id()}>" | |
else: | |
return f"<{self.__class__.__name__}>" |
Just to illustrate this, here's a full list (I think) of objects, a lot of them would benefit from this feature. I don't want you to do this for all of them, but if you could try to add it for every type of *Project
we have a good start to check it works. If it's too repetitive, we could introduce mixins for this at some point, or some other more reusable way if you have any ideas :) WDYT @Psycojoker @JohnVillalovos
Application
ApplicationAppearance
ApplicationSettings
AuditEvent
BroadcastMessage
CurrentUser
CurrentUserEmail
CurrentUserGPGKey
CurrentUserKey
CurrentUserStatus
DeployKey
DeployToken
Dockerfile
Event
Feature
GenericPackage
GeoNode
Gitignore
Gitlabciyml
Group
GroupAccessRequest
GroupAccessToken
GroupAuditEvent
GroupBadge
GroupBillableMember
GroupBillableMemberMembership
GroupBoard
GroupBoardList
GroupCluster
GroupCustomAttribute
GroupDeployToken
GroupDescendantGroup
GroupEpic
GroupEpicAwardEmoji
GroupEpicDiscussionNote
GroupEpicIssue
GroupEpicNote
GroupEpicNoteAwardEmoji
GroupEpicResourceLabelEvent
GroupExport
GroupHook
GroupImport
GroupIssue
GroupIssuesStatistics
GroupLabel
GroupMember
GroupMemberAll
GroupMergeRequest
GroupMilestone
GroupNotificationSettings
GroupPackage
GroupProject
GroupRunner
GroupSubgroup
GroupVariable
GroupWiki
Hook
Issue
IssuesStatistics
Key
LDAPGroup
License
List
MergeRequest
Namespace
NotificationSettings
Optional
PagesDomain
PersonalAccessToken
Project
ProjectAccessRequest
ProjectAccessToken
ProjectAdditionalStatistics
ProjectApproval
ProjectApprovalRule
ProjectArtifact
ProjectAudit
ProjectAuditEvent
ProjectBadge
ProjectBoard
ProjectBoardList
ProjectBranch
ProjectCluster
ProjectCommit
ProjectCommitComment
ProjectCommitDiscussion
ProjectCommitDiscussionNote
ProjectCommitStatus
ProjectCustomAttribute
ProjectDeployToken
ProjectDeployment
ProjectDeploymentMergeRequest
ProjectEnvironment
ProjectEvent
ProjectExport
ProjectFile
ProjectFork
ProjectHook
ProjectImport
ProjectIssue
ProjectIssueAwardEmoji
ProjectIssueDiscussion
ProjectIssueDiscussionNote
ProjectIssueLink
ProjectIssueNote
ProjectIssueNoteAwardEmoji
ProjectIssueResourceLabelEvent
ProjectIssueResourceMilestoneEvent
ProjectIssueResourceStateEvent
ProjectIssuesStatistics
ProjectJob
ProjectKey
ProjectLabel
ProjectMember
ProjectMemberAll
ProjectMergeRequest
ProjectMergeRequestApproval
ProjectMergeRequestApprovalRule
ProjectMergeRequestApprovalState
ProjectMergeRequestAwardEmoji
ProjectMergeRequestDiff
ProjectMergeRequestDiscussion
ProjectMergeRequestDiscussionNote
ProjectMergeRequestNote
ProjectMergeRequestNoteAwardEmoji
ProjectMergeRequestPipeline
ProjectMergeRequestResourceLabelEvent
ProjectMergeRequestResourceMilestoneEvent
ProjectMergeRequestResourceStateEvent
ProjectMergeTrain
ProjectMilestone
ProjectNote
ProjectNotificationSettings
ProjectPackage
ProjectPackageFile
ProjectPagesDomain
ProjectPipeline
ProjectPipelineBridge
ProjectPipelineJob
ProjectPipelineSchedule
ProjectPipelineScheduleVariable
ProjectPipelineTestReport
ProjectPipelineTestReportSummary
ProjectPipelineVariable
ProjectProtectedBranch
ProjectProtectedTag
ProjectPushRules
ProjectRegistryRepository
ProjectRegistryTag
ProjectRelease
ProjectReleaseLink
ProjectRemoteMirror
ProjectRunner
ProjectService
ProjectSnippet
ProjectSnippetAwardEmoji
ProjectSnippetDiscussion
ProjectSnippetDiscussionNote
ProjectSnippetNote
ProjectSnippetNoteAwardEmoji
ProjectTag
ProjectTrigger
ProjectUser
ProjectVariable
ProjectWiki
RepositoryMixin
Runner
RunnerJob
Snippet
StarredProject
Todo
Topic
User
UserActivities
UserCustomAttribute
UserEmail
UserEvent
UserGPGKey
UserImpersonationToken
UserKey
UserMembership
UserPersonalAccessToken
UserProject
UserStatus
Variable
About pre-commit - sorry to hear that. Generally the idea of We kind of do need pre-commit or something like that if we keep adding more linters and nitpicky rules though. Otherwise people will push a lot of failing code and it creates more noise for followers and extra effort, especially with new contributors where every push needs approval for CI to run. The other option is that we calm down with the linting a bit and make things looser. But the commit messages at least are nice, as it makes our release process really smooth and we can bring out new versions much quicker if needed :) |
I've started #1998 to address the pre-commit issue. We can also just merge this as is and work on refactoring after, just let us know @Psycojoker if you'd like to try yourself already. :) |
Ok, so let's get this in to have the feature and I'll rework it based on my comment in a follow-up. |
This change the repr from:
To:
This is especially useful when working on random projects or listing of
projects since users generally don't remember projects ids.
On a side note I've encountered this error when trying to do a commit:
I have to admit that, while I totally understand the importance of linters and good commit messages, having a hard dependency on a specific version of nodejs and glibc just do to a commit on a full python feels a bit ... intense? 😅
Anyway, thanks a lot for this project ❤️