search

WMEValidator / validator

WME Validator Source Files
GNU General Public License v3.0
11 stars 10 forks source link

[Venue] Adding Google place link checks #74

Closed davidakachaos closed 5 years ago

davidakachaos commented 5 years ago

Took these checks from WME Utils - Google Link Enhancer Trying to use the same cache (for when people have that script installed)

berestovskyy commented 5 years ago

Do you think it is a good case for #58 (custom plug-ins)? The Google Link Enhancer is already available as a library, it has the same license. Why don't we try to create a generic interface for such libraries in Validator?

davidakachaos commented 5 years ago

Yes it is, although most (all?) check could be converted to a plugin system maybe. It would take an approach such as Wide Angle Lens (another WME script)

How would make such a generic interface? Do we base it on the existing way we load external language packs (src/login.js:434) ? I think that would be at least an starting point.

berestovskyy commented 5 years ago

The journey of a thousand miles begins with one step ;) We shall not convert all the checks right now, but we shall not integrate a ready to use plug-in into the codebase. Instead, we might try to create an interface in Validator to use the plugins like this one.

I had another look at the plugin interface, and, unfortunately, this plugin cannot be used as is, since it integrates into WME interface etc. Maybe @justins83 could provide his opinion, what would be the best option to integrate Validator and Google Link Enhancer?

justins83 commented 5 years ago

Hey guys, I apologize for not being around...I got pretty burnt out and have really needed the break. I'm not quite ready to dive into this project (not really making progress on any projects right now) but it is great seeing all the activity from David getting things working.

Re: GLE - It was designed to be a library that can be instantiated and integrate with WME but I think it could be adapted such that validator could instantiate and instance and then call a method to do the checking against a Place's external links either by passing them individually or passing the Place object and the script looping through all of the external links.

I wouldn't suggest extracting the code from the library for incorporating into Validator...that's the entire point of having libraries :)

davidakachaos commented 5 years ago

Thanks @justins83 I'm trying my best. And taking time for yourself to prevent a total burnout is important! So don't worry too much, we try to keep the script in good condition.

I think you make a real good point that we should not copy the code, but find another way to integrate the library. I think we should close this PR for now and look more in the direction of creating check plugins for Validator. Let the plugin "worry" about getting GLE into the mix.

All agree to close this for now?

berestovskyy commented 5 years ago

Unfortunately, the GLE needs to be refactored, we will not be able to use it as is. So I guess the way to go is to use this code or the original GLE as a basis for a new plug-in.

The original GLE code uses Promises, i.e. it is async. Sorry, I did not test the change, David, do you see any UX (i.e. WME responsiveness) degradation with this sync version? I guess it might be noticeable for places with lots of external providers.

Overall, performance degradation is my biggest concern. I would prefer to have this functionality as a plug-in, so users could switch it off any time.

Let us merge this pull request on a separate branch for future reference. We might build a new plugin using this code...

davidakachaos commented 5 years ago

I did not notice a lot of UX degradation, this could be because I already have the GLE running (part of another addon I have, searching which one exactly...) and the cache of it was already filled with some links.

Performance is always an issue, and I think when we open Validator for plugins to do their own checks, we will see someone break the performance of the main script in some way. We should look into ways maybe to execute the plugins async to let them resolve in their own "threads" while the main script waits for them to resolve.

For now I'll leave this hanging a bit (or you can merge it to its own branch) and still have the code in its own branch on my fork and this PR.