Skip to content

Change all bug ID to links#7441

Merged
dlang-bot merged 5 commits intodlang:masterfrom
Geod24:bugzilla-links
Apr 14, 2020
Merged

Change all bug ID to links#7441
dlang-bot merged 5 commits intodlang:masterfrom
Geod24:bugzilla-links

Conversation

@Geod24
Copy link
Member

@Geod24 Geod24 commented Apr 12, 2020

Context: There has been work done to move towards Github, but concerns have been raised that it would make issue number ambiguous. In order to avoid this undesirable side effect, this PR converts most issue number (I might have missed a few, but I doubt that there's many) into HTTP links.

See also: dlang/project-ideas#43
CC @CyberShadow @WalterBright

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

Bugzilla references

Auto-close Bugzilla Severity Description
5710 normal cannot use delegates as parameters to non-global template

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7441"

Copy link
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like such large changes should be held off until we definitely know for sure that 1) we want to move to GitHub and 2) we cannot preserve issue numbers while doing so.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In either way this is a very welcome change. Thanks a lot!
BTW for DMD Walter enforces that issues can only be referenced by links stricter and I do believe the same reasoning (clickable, unmistakable references) applies here as well.

@CyberShadow
Copy link
Member

and I do believe the same reasoning (clickable, unmistakable references) applies here as well.

Good point.

@CyberShadow CyberShadow dismissed their stale review April 12, 2020 22:34

"A message or commit OID is required to dismiss a review"

@Geod24
Copy link
Member Author

Geod24 commented Apr 13, 2020

Fixed the style issue.
Indeed should have mentioned the precedent set by Walter: dlang/dmd#6775

Make the links clickable, as was done in the DMD repository.
Also avoids any ambiguity w.r.t. where the issue is stored.
Copy link
Member

@burner burner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very boring work, I like it

@Geod24 Geod24 requested a review from thewilsonator April 14, 2020 12:28
@Geod24
Copy link
Member Author

Geod24 commented Apr 14, 2020

So... Can we move forward with this before it becomes stale ?

@CyberShadow
Copy link
Member

CyberShadow commented Apr 14, 2020

Thank you! This looks like a lot of tedious work.

Copy link
Member

@schveiguy schveiguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, still looking, I'll leave as a comment because technically, the original was wrong, but an invalid url sucks more than a number.

@Geod24
Copy link
Member Author

Geod24 commented Apr 14, 2020

Added a commit to correct the issue number, thanks @MoonlightSentinel

@Geod24
Copy link
Member Author

Geod24 commented Apr 14, 2020

@schveiguy : Thanks for doing that tedious review! LMK when you're done, I'll fix and reapply auto-merge.

@CyberShadow
Copy link
Member

I suggest sending follow-up fixes as new PRs to avoid retriggering the CIs and increasing the chances of merge failures.

@schveiguy
Copy link
Member

I agree, just merge this. I'm 37/69 files reviewed. Anything I find can be fixed in a smaller update.

Thanks for doing that tedious review!

Some of it github makes easy by highlighting just the changed portions of a line, and of course being able to mark files as viewed. But this doesn't compare to the tediousness of actually making this fix. Thanks!

Copy link
Member

@schveiguy schveiguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done the review, let's merge and then do fixups after.

@dlang-bot dlang-bot merged commit ffca395 into dlang:master Apr 14, 2020
@Geod24 Geod24 deleted the bugzilla-links branch April 14, 2020 15:42
@schveiguy
Copy link
Member

A recommendation for future similar PRs, to make this much easier to review -- One commit that simply replaces the number with the link. No format editing, no removals of outdated anything. Just a simple replacement. This enables one to review simply that the numbers are not changed (that was my main focus). Github makes this easy by highlighting the only the changed portion of the text. Then more commits to fix the formatting, etc.

@Geod24
Copy link
Member Author

Geod24 commented Apr 14, 2020

Right. I originally didn't expect it to be so big, but after the initial (scripted) madness, I realized that the style was wildly inconsistent (e.g. references used as well as how things were organized) and had to go in manually and fix tons of stuff.
For reference, there was an unavoidable amount of stylistic changes needed, because the style linter complains about long lines (triggered by @some_attributes unittest // here goes the long URL and some description) and some patterns, e.g. it enforces allman but relies on { being at EOL to allow void foo() { }. But this also allow void foo() { // Oops.

But point taken, I'll keep the future changes much shorter.

@Geod24
Copy link
Member Author

Geod24 commented Apr 14, 2020

Followed up on: #7446
Thanks for the careful review!

@schveiguy
Copy link
Member

You may misunderstand, I mean separate the adding of the urls from the formatting in 2 commits, not 2 PRs. This shouldn't affect the style checker.

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.

7 participants