Skip to content

[GiteaBridge] Add Tags context#2670

Closed
gileri wants to merge 3 commits intoRSS-Bridge:masterfrom
gileri:gitea-tags
Closed

[GiteaBridge] Add Tags context#2670
gileri wants to merge 3 commits intoRSS-Bridge:masterfrom
gileri:gitea-tags

Conversation

@gileri
Copy link
Contributor

@gileri gileri commented Apr 17, 2022

Following feedback in #2069, here is a new PR, with a more limited scope.

Add support for tags context.

@github-actions
Copy link

github-actions bot commented Apr 17, 2022

Pull request artifacts

file last change
Gitea-current-context1 2022-05-09, 18:59:55
Gitea-current-context2 2022-05-09, 18:59:55
Gitea-current-context3 2022-05-09, 18:59:55
Gitea-current-context4 2022-05-09, 18:59:55
Gitea-pr-context1 2022-05-09, 18:59:55
Gitea-pr-context2 2022-05-09, 18:59:55
Gitea-pr-context3 2022-05-09, 18:59:55
Gitea-pr-context4 2022-05-09, 18:59:55
Gitea-pr-context5 2022-05-09, 18:59:55

@Bockiii
Copy link
Contributor

Bockiii commented Apr 19, 2022

This makes more sense in the gogsbridge. Can you redo the functionality for the gogsbridge and add changes to the gitea bridge if neccessary? I dont know if gogs-tags work different to gitea-tags. It's hard to find any public instances with actual data and releases.

@gileri
Copy link
Contributor Author

gileri commented Apr 25, 2022

Hello Bockiii,

If you follow the link in the OP, you'll see that this feature was implemented in Gitea, independently of Gogs.

You can compare both implementation of tags here :

I don't think there is enough in common between the two to warrant a solution that works for both, implemented in GogsBridge.

@Bockiii
Copy link
Contributor

Bockiii commented Apr 26, 2022

We at least need to remove all the duplicate code. Right now you basically copied the whole gogs bridge into the gitea bridge.

@dvikan
Copy link
Contributor

dvikan commented May 8, 2022

I'm not a big fan of inheritance. But if we follow bockiis advice and do it properly, it should be done like this:

diff --git a/bridges/GiteaBridge.php b/bridges/GiteaBridge.php
index 3324787..25165aa 100644
--- a/bridges/GiteaBridge.php
+++ b/bridges/GiteaBridge.php
@@ -24,4 +24,17 @@ class GiteaBridge extends GogsBridge {
 			);
 		}
 	}
+
+	protected function collectTags($html)
+	{
+		$tags = $html->find('table#tags-table > tbody > tr')
+		or returnServerError('Unable to find tags');
+
+		foreach($tags as $tag) {
+			$this->items[] = array(
+				'uri' => $tag->find('a', 0)->href,
+				'title' => 'Tag ' . $tag->find('.release-tag-name > a', 0)->plaintext,
+			);
+		}
+	}
 }
diff --git a/bridges/GogsBridge.php b/bridges/GogsBridge.php
index a2adc1f..0167dab 100644
--- a/bridges/GogsBridge.php
+++ b/bridges/GogsBridge.php
@@ -53,6 +53,7 @@ class GogsBridge extends BridgeAbstract {
 			),
 		),
 		'Releases' => array(),
+		'Tags' => array(),
 	);
 
 	private $title = '';
@@ -98,6 +99,7 @@ class GogsBridge extends BridgeAbstract {
 			case 'Issues':
 			case 'Releases': return $this->title . ' ' . $this->queriedContext;
 			case 'Single issue': return $this->title . ' Issue ' . $this->getInput('issue');
+			case 'Tags': return 'Tags for ' . $this->title;
 			default: return parent::getName();
 		}
 	}
@@ -127,6 +129,9 @@ class GogsBridge extends BridgeAbstract {
 			case 'Releases': {
 				$this->collectReleasesData($html);
 			} break;
+			case 'Tags':
+				$this->collectTags($html);
+				break;
 		}
 
 	}
@@ -202,4 +207,9 @@ class GogsBridge extends BridgeAbstract {
 			);
 		}
 	}
+
+	protected function collectTags($html)
+	{
+		// Not implemented for gogs
+	}
 }

@gileri
Copy link
Contributor Author

gileri commented May 9, 2022

We at least need to remove all the duplicate code

The code is duplicated for a reason : the projects are drifting away more and more, and some features are not available in Gogs, or Gitea (e.g. the Tags page). We could do more convoluted inheritance between the two bridges, but to what end ? Both bridges are simple enough anyway.

But if we follow bockiis advice and do it properly, it should be done like this

@dvikan Thank you for suggesting an implementation ! However doing it this way makes the Tags context available in the GogsBridge in the web UI, making users think it's implemented. Is using inheritance more important than presenting the correct available contexts to users ? I think we away with it, especially if you're not a fan of it too ;)

Also your comment made me realize that the Bridge did not have a name/title for the Tags context, that's fixed in f758993.

@dvikan
Copy link
Contributor

dvikan commented May 9, 2022

Your comments make sense gilleri. I think I prefer duplication here. So let's do it properly. Duplicate the bridge, drop the inheritance and implement the desired changes.

@Bockiii
Copy link
Contributor

Bockiii commented May 10, 2022

I'm not a big fan of inheritance either, but if we do it, we should do it correctly :)

So yes, if we assume that both projects are drifting away further, please rewrite the gitea bridge so it doesn't inherit anything and just stands on it's own. Worst case scenario would be a change that needs to be done in both bridges, that's doable.

@gileri
Copy link
Contributor Author

gileri commented May 10, 2022

I agree, thanks for your support. I've rewritten completely GiteaBridge to decouple it from GogsBridge, and will open a new PR, since this one strayed quite far from the original goal.

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.

3 participants