Adding configuration and new checks #73

davidakachaos · 2018-10-08T08:42:00Z

Added some new checks and a configuration option.
Sorry for not splitting this up correctly, if you want I could try to break this apart.

Master switch if a user wants to check venues or not.

berestovskyy · 2018-10-08T22:25:35Z

src/validate.js

@@ -545,11 +545,14 @@ function F_VALIDATE(disabledHL) {
 			this.$isApproved = attrs.approved;
 			this.$mainCategory = raw.getMainCategory();
 			this.$categories = attrs.categories;
+			this.$categoryAttributes = attrs.categoryAttributes;


Do we need to add categoryAttributes to the externs?

Agreed, it should be

berestovskyy · 2018-10-08T22:33:38Z

src/validate.js

-				seen = null;
+			if ("Delete" === rawVenue.state) continue;
+			// not in scope of current view.
+			if (rawVenue.outOfScope && rawVenue.outOfScope === true) continue;


Just if (rawVenue.outOfScope) continue;?

berestovskyy · 2018-10-09T07:09:15Z

src/validate.js

+			var alts = venue.$alts;
+			var lock = venue.$lock;
+
+			var exceptedCategories ='BRIDGE|ISLAND|FOREST_GROVE|SEA_LAKE_POOL|RIVER_STREAM|CANAL|DAM|TUNNEL|JUNCTION_INTERCHANGE'.split('|');


No hard-coded values should be in the code. Those categories should be moved either to i18n as a default value or to the data.js.

berestovskyy · 2018-10-09T07:14:12Z

src/validate.js

+				'^admin$', '^-1$',
+				'^avsus$', '^107668852$'
+			];
+			var re = new RegExp(botNamesAndIDs.join('|'),'i');


Same here. The Regex should be created during the initialization. Looks like it belongs to i18n.

berestovskyy · 2018-10-09T07:23:56Z

src/validate.js

+					&& address.isOkFor(254))
+					venue.report(254);
+				//check elevation of the parking lot
+				if ((!parkAttr || !parkAttr.lotType || parkAttr.lotType.length === 0)


The checks !parkAttr.lotType and parkAttr.lotType.length === 0 are the same

lotType can be undefined or {} is that both caught by either check? I thought I needed both checks?

Any value that is not false, undefined, null, 0, NaN, or the empty string (""), and any object, including a Boolean object whose value is false, is considered truthy when used as the condition.

Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/if...else

If lotType is an object, I guess there will be no length property there, unless it is added by WME. Do you happen to have a permalink with a parking lot at hand, since there are no parking lots around...

Sure here; parking lot with things set corectly
and one with less things set

In the correctly set permalink I see the lotType is an array and it is empty. Sorry, we need to check if the array length is zero, but I am not quite sure why it is empty in the correctly set PL?

Empty?

I see it with one element in it ("STREET_LEVEL")

Sorry, I might have missed the venue. We need to check the length of the array [] is not zero just as you had it before.

berestovskyy · 2018-10-09T07:31:05Z

src/validate.js

+			if (venue.$rawObject.isGasStation()) {
+				// check if brand in name
+				if (venue.$name.indexOf(venue.$brand) === -1
+					&& isLimitOk(259)


Overall, it is not necessary to use isLimitOk(), since the venue.report() below will check the limit anyway.

The rationale behind isLimitOk() is to avoid long computations. For example:

if (isLimitOk()) { // long check if (condition) report();

}

In such cases it makes sense, since if the limit is exceeded, we do not even start the "long check" part.

Ah, okay, that makes sense. I need to review the checks then. Thanks for the explanation.

berestovskyy · 2018-10-09T07:58:13Z

src/validate.js

+
+			if(venue.$entryExitPoints && venue.$entryExitPoints.length){
+				var stopPoint = venue.$entryExitPoints[0].getPoint();
+				var areaCenter = venue.$geometry.getCentroid();


Not quite understand this check. And it seems there is no commit in i18n?

This checks if the entry/exit point of a place has not been moved from its original position when created (equal to the center of the venue) I'll check the i18n, 256 should be there or I've forgotten it

berestovskyy · 2018-10-10T19:36:05Z

Looks good. Do not need to squash yet, we will squash on merge to the master.

I will test the dev branch during the weekend. Maybe we should ask for some volunteers on the forum to test this branch and provide a feedback as well?

davidakachaos · 2018-10-11T07:33:20Z

Okay, we will merge and do a test. Maybe we should put up a test version pre build somewhere, easier for people to test that do not know (or want) how to build.
We could put the script unlisted on GreasyFork and link it on the forum for people to test. If we do that, we can ask people to test the new functions on the forum, discord, slack etc...

berestovskyy · 2018-10-11T09:25:33Z

Would you like to lead this beta branch?

davidakachaos · 2018-10-15T07:39:53Z

Sorry, had my kids over the weekend so was offline for most of it 😉
I'm willing to help out anyway I can, so yeah, I'll set something up so we can spread it around.

davidakachaos · 2018-10-15T10:15:54Z

@berestovskyy I've created a beta version of the script and put it on GreasyFork as unlisted here

I want to link it on the forum as a test version using the following text:

Hello everyone! We've created a test version of WME Validator with support for checking venues beside segments. We want to test this out and see if everything works correctly. We've some new checks specially for venues. We would love to hear your feedback on this either here on the forum or via the Github issues! If you're willing to test this version out, please install the beta version of this script via this link (be sure to disable the current install of WME Validator!)

This could be posted to the discord too. Would this be okay?

berestovskyy · 2018-10-15T16:13:57Z

Sorry, I just realized I have added the beta tag to the main version of Validator :) To avoid confusions, maybe let's call it alpha, experimental, release candidate (RC), daily build, or anything different than beta?

And it would be nice if I or potentially other developers could update this experimental branch as well. I just googled if it's possible to share a script on GF, but it seems not:
greasyfork-org/greasyfork#311

Maybe we better create a repo similar to release:
https://github.com/WMEValidator/release

So we have a bit more flexibility with access permissions. What do you think?

justins83 · 2018-10-15T18:01:39Z

You can sync a github repository to a greasyfork script via webhook, therefore not needing to share the greasyfork credentials. This is the simplest way to do it and what we have done in the WazeDev group

…

On Mon, Oct 15, 2018 at 12:14 PM Andriy Berestovskyy < ***@***.***> wrote: Sorry, I just realized I have added the beta tag to the main version of Validator :) To avoid confusions, maybe let's call it alpha, experimental, release candidate (RC), daily build, or anything different than beta? And it would be nice if I or potentially other developers could update this experimental branch as well. I just googled if it's possible to share a script on GF, but it seems not: greasyfork-org/greasyfork#311 <greasyfork-org/greasyfork#311> Maybe we better create a repo similar to release: https://github.com/WMEValidator/release So we have a bit more flexibility with access permissions. What do you think? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#73 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATTy2vcXCiU8g6NsnyUQGzt0AAmiSzwNks5ulLRGgaJpZM4XMedV> .

-- JustinS83 USA Local Champ

davidakachaos · 2018-10-15T23:25:55Z

@berestovskyy I'll rename it to alpha then 😄 And I agree, it would be nice to have.
@justins83 We have a build script that needs to be run before we publish. Maybe we need to look into a auto build service? And publish from there?

justins83 · 2018-10-16T01:25:26Z

Yes, I understand how the build process works. It should be trivial to add to the build script to commit the changes to github once complete, which would then trigger the webhook and posting to greasyfork.

…

On Mon, Oct 15, 2018 at 7:25 PM David Westerink ***@***.***> wrote: @berestovskyy <https://github.com/berestovskyy> I'll rename it to alpha then 😄 And I agree, it would be nice to have. @justins83 <https://github.com/justins83> We have a build script that needs to be run before we publish. Maybe we need to look into a auto build service? And publish from there? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#73 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATTy2oxV_53ILrycFLyTVTg0r0bcIvDuks5ulRmEgaJpZM4XMedV> .

-- JustinS83 USA Local Champ

berestovskyy · 2018-10-16T05:39:35Z

IMO a webhook or an automatic sync does not change the fact that we need a shared account on GreasyFork, or do I miss something?

justins83 · 2018-10-16T12:54:05Z

You can certainly set up an account and share the credentials, but how many greasyfork scripts do you really need to set up? Production, beta and maybe alpha? Once those are set up and the webhooks established why would someone need continued access to the account?

…

On Tue, Oct 16, 2018 at 1:39 AM Andriy Berestovskyy < ***@***.***> wrote: IMO a webhook or an automatic sync does not change the fact that we need a shared account on GreasyFork, or do I miss something? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#73 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATTy2vUD18Tf33sfD7ZP3XqrMXoA4XYIks5ulXEYgaJpZM4XMedV> .

-- JustinS83 USA Local Champ

berestovskyy · 2018-10-16T15:55:03Z

Still, someone has to have an access... Anyway, seems David has decided to post the script under his account, so it does not matter anymore ;)

davidakachaos · 2018-10-16T15:59:56Z

I would be a better way to do it, but I wanted to start testing things out soon. Sorry if I made a mistake in that :-/

berestovskyy · 2018-10-16T20:20:08Z

It's ok with me. We can always create another GitHub repo or a shared account on GreasyFork...

davidakachaos added 2 commits October 8, 2018 09:57

Added new venue checks

8354aea

Adding configuration option for venues

96a3f19

Master switch if a user wants to check venues or not.

davidakachaos requested a review from berestovskyy October 8, 2018 08:42

berestovskyy reviewed Oct 9, 2018

View reviewed changes

davidakachaos added 5 commits October 9, 2018 10:29

Adding categoryAttributes to the wme-externs

f5a320d

Optimized checks

8fd4a4a

Adjusted check after review

c812168

Revert fix on check lotType

c5ceeea

Fix syntax error (will squash on merge)

7d5e31d

davidakachaos merged commit 0505a02 into WMEValidator:dev Oct 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding configuration and new checks #73

Adding configuration and new checks #73

davidakachaos commented Oct 8, 2018

berestovskyy Oct 8, 2018

davidakachaos Oct 9, 2018

berestovskyy Oct 8, 2018

davidakachaos Oct 9, 2018

berestovskyy Oct 9, 2018

berestovskyy Oct 9, 2018

berestovskyy Oct 9, 2018

davidakachaos Oct 9, 2018

berestovskyy Oct 9, 2018

berestovskyy Oct 9, 2018

davidakachaos Oct 9, 2018

davidakachaos Oct 9, 2018

berestovskyy Oct 9, 2018

davidakachaos Oct 9, 2018

berestovskyy Oct 9, 2018

berestovskyy Oct 9, 2018

davidakachaos Oct 9, 2018

berestovskyy Oct 9, 2018

davidakachaos Oct 9, 2018

berestovskyy commented Oct 10, 2018

davidakachaos commented Oct 11, 2018

berestovskyy commented Oct 11, 2018

davidakachaos commented Oct 15, 2018

davidakachaos commented Oct 15, 2018

berestovskyy commented Oct 15, 2018 •

edited

Loading

justins83 commented Oct 15, 2018 via email

davidakachaos commented Oct 15, 2018

justins83 commented Oct 16, 2018 via email

berestovskyy commented Oct 16, 2018

justins83 commented Oct 16, 2018 via email

berestovskyy commented Oct 16, 2018

davidakachaos commented Oct 16, 2018

berestovskyy commented Oct 16, 2018

Adding configuration and new checks #73

Adding configuration and new checks #73

Conversation

davidakachaos commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berestovskyy commented Oct 10, 2018

davidakachaos commented Oct 11, 2018

berestovskyy commented Oct 11, 2018

davidakachaos commented Oct 15, 2018

davidakachaos commented Oct 15, 2018

berestovskyy commented Oct 15, 2018 • edited Loading

justins83 commented Oct 15, 2018 via email

davidakachaos commented Oct 15, 2018

justins83 commented Oct 16, 2018 via email

berestovskyy commented Oct 16, 2018

justins83 commented Oct 16, 2018 via email

berestovskyy commented Oct 16, 2018

davidakachaos commented Oct 16, 2018

berestovskyy commented Oct 16, 2018

berestovskyy commented Oct 15, 2018 •

edited

Loading