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

[Multiple] Fix all exampleValues and required variables #2296

Merged
merged 16 commits into from
Mar 24, 2022

Conversation

Bockiii
Copy link
Contributor

@Bockiii Bockiii commented Oct 8, 2021

Hi,

I went through the bridges and added/fixed all exampleValues and required statements for all bridges.

This PR is probably not done right now and is WIP after I'm done testing.

@Bockiii
Copy link
Contributor Author

Bockiii commented Oct 8, 2021

Ouh oh... i didnt check that lists cant have a required... crappo.. I will need to check what this changes.

@em92 em92 marked this pull request as draft October 9, 2021 10:30
@somini
Copy link
Contributor

somini commented Oct 9, 2021

slow clap

From what I can tell there's no real loss of information here. Great job, LGTM.

bridges/CodebergBridge.php Outdated Show resolved Hide resolved
bridges/IdenticaBridge.php Outdated Show resolved Hide resolved
bridges/TwitterBridge.php Outdated Show resolved Hide resolved
@yamanq
Copy link
Contributor

yamanq commented Oct 17, 2021

LGTM, just a couple changes :)

)
),
'User Wantlist' => array(
'username_wantlist' => array(
'name' => 'Username',
'type' => 'text',
'required' => true,
'exampleValue' => 'TheBlindMaster',
)
),
'User Folder' => array(
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://www.discogs.com/developers/#page:user-collection,header:user-collection-collection-folder

It looks like only folderid = 0 is accessible to us at this time. Since this PR is focused only on the default/example values, the best settings for this context are:

		'User Folder' => array(
			'username_folder' => array(
				'name' => 'Username',
				'type' => 'text',
				'required' => true,
				'exampleValue' => 'TheBlindMaster'
			),
			'folderid' => array(
				'name' => 'Folder ID',
				'type' => 'number',
				'required' => true,
				'defaultValue' => 0
			)
		)

@@ -9,15 +9,17 @@ class HDWallpapersBridge extends BridgeAbstract {
const PARAMETERS = array( array(
'c' => array(
'name' => 'category',
'required' => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This bridge doesn't require a category, but I think latest_wallpapers is equivalent to blank category. Either way, it's a small issue since the bridge only returns broken links.

@@ -9,6 +9,7 @@ class IKWYDBridge extends BridgeAbstract {
array(
'ip' => array(
'name' => 'IP Address',
'exampleValue' => '8.8.8.8',
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that your tests showed that 8.8.8.8 returned a list of 2. I get an empty feed and I'm not sure why.

This IP returns a non-empty feed and the ip seems to belong to reliablehosting.com (a non-personal company)
We can also choose other examples from here: https://iknowwhatyoudownload.com/en/peers/?id=3f69ea4d38f96aa93746a68f5af33dd9d51c7c30343f33f71677a2c7157a79726547f5dabbe7145996dc298fc91a02a9&country=US

Suggested change
'exampleValue' => '8.8.8.8',
'exampleValue' => '216.131.86.165',

@dvikan
Copy link
Contributor

dvikan commented Nov 28, 2021

Wow this is really nice piece of work. Make it much easier for users to test a bridge. Thank you.

@Bockiii
Copy link
Contributor Author

Bockiii commented Nov 30, 2021

@em92 I would love to have this merged as it is. The leftover comments are more about flavor and can be done in their own PRs if someone wants to. They don't effect the validity of this PR and merging this will help a lot with the progress of the bridge tester.

@em92 em92 marked this pull request as ready for review December 10, 2021 05:14
@dvikan
Copy link
Contributor

dvikan commented Dec 11, 2021

Can this be merged please? Excellent work by @Bockiii

@Bockiii
Copy link
Contributor Author

Bockiii commented Dec 11, 2021

The problem is: the longer we wait, the more merge conflicts could come up.

@f0086
Copy link
Contributor

f0086 commented Dec 20, 2021

Awesome, thank you @Bockiii for your work!
Please merge this!

@dvikan
Copy link
Contributor

dvikan commented Mar 21, 2022

Please merge this @em92

Will make it easier to continue the work of automatic testing of bridges.

@Bockiii Bockiii mentioned this pull request Mar 22, 2022
@Bockiii
Copy link
Contributor Author

Bockiii commented Mar 24, 2022

Alright, the only linting error is actually solved by #2521 so it doesnt concern me.

I will add another small PR to re-add the examplevalue on the giphy bridge (didnt want to deal with the merge conflict because giphy was updated in the mean time.. this is easier.
comics was changed and received an examplevalue
ettv, tagboard and tvdb bridges were all removed.

@Bockiii Bockiii merged commit 1a8d0ba into RSS-Bridge:master Mar 24, 2022
Kwbmm pushed a commit to Kwbmm/rss-bridge that referenced this pull request Jun 17, 2022
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.

6 participants