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

Improve clarity for referenced and cross-referenced timeline events #1182

Merged
merged 4 commits into from
Feb 20, 2022

Conversation

Fs00
Copy link
Contributor

@Fs00 Fs00 commented Feb 19, 2022

This PR makes the following improvements to the way referenced and cross-referenced timeline events are displayed:

  • referenced: display also repository owner and name together with the SHA of the commit that referenced the issue/PR, if it belongs to another repository.
    I've also reworded the message for the event since I've discovered that references from commit comments (and not only from commit messages) appear as referenced events too (see this for an example), and therefore the message "added commit [] that referenced this issue" could have been misleading.
  • cross-referenced: display also repository owner and name together with the number of the issue that referenced the issue/PR, if it belongs to another repository (like GitHub does, e.g. dotnet/runtime#12456).

I've also extracted a few methods to improve code readability.

@maniac103
Copy link
Collaborator

In general this LGTM, but...

I've also extracted a few methods to improve code readability.

... I have to admit I'm not a fan of such one-off methods, because it makes one jump back-and-forth through the code when following the logic.
Since each of those pieces of code is in its own block due to the indexOf check there's no improvement in variable scoping, and there's no dependency between them so I honestly don't see the improvement here.

@Fs00
Copy link
Contributor Author

Fs00 commented Feb 20, 2022

I think there are a few compelling reasons to extract those sections in separate methods:

  • the formatEvent() method is quite daunting (220 lines of code before the refactoring) and it's hard to get an overview of what's doing by just peeking at the code.
  • when I first looked at the code, I had to spend some time parsing a few of those sections to understand that they were substituting a placeholder in the text. By extracting them and giving them a name, it's easier to understand what they are doing without having to get into the details of the logic.
  • "there's no dependency between them": true, and it sounds like a very good reason to extract them to me, because in this way you enforce the decoupling but most importantly you make it very explicit (it's not very visible when you read the code the first time).

About the jumping back-and-forth, and I don't think that's the case here. The details of those sections aren't really important to know if you're just following the logic, and furthermore it seems to me that the sections are changed independently of each other in most cases (so it's unlikely you'll need to jump around), and having each of them extracted helps (me at least) focusing on one piece at a time.
Also, you can see from the diff of commit 6d188a8 that the code wasn't moved around much, the main changes are the method declarations being added in between the code.

Just to be clear, I don't want to impose my way of structuring code here 😅, my goal was just to avoid to others the struggle I've had when first approaching the code.
It's also fine for me if you prefer to revert the refactoring.

@maniac103
Copy link
Collaborator

How about adding comments as a (meaningful) compromise?

@Fs00
Copy link
Contributor Author

Fs00 commented Feb 20, 2022

Definitely better than nothing for someone reading the code for the first time, but you won't get the aforementioned benefits about explicit scoping, "hiding" unnecessary details when just following the logic, and making the method size more manageable.
Also, I personally prefer when the code itself tells what it's doing, rather than the human using comments (unless when needed to explain motivations or reasonings behind the code).

@maniac103 maniac103 merged commit d5dc26e into slapperwan:master Feb 20, 2022
@Fs00 Fs00 deleted the ref-event branch February 20, 2022 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants