[19:42:45] legoktm: I'm looking to fix https://phabricator.wikimedia.org/T126091 - but don't want to introduce a global variable for it, so basically attribute-only, and then have RL call that inaddition to the hook, that wouldn't be weird, right? [19:43:35] The only reason it's a hook was to reduce run-time overhead for prod, but as plain JSON that seems not worth optimising [19:46:37] Krinkle: yeah, attribute-only makes sense [19:46:45] Krinkle: we do that in EventLogging (except that has a legacy global too) [19:46:50] legoktm: I'm trying to figure out which bucket that b... [19:46:53] OK, I'll look at that. [19:47:12] Krinkle: ❤️ [19:47:15] globalSetting , coreAttribute, notAttributes [19:47:18] coreAttributes sounds right? [19:47:23] yep [19:48:31] legoktm: but in the actual json files, that'd be a sibling of a global setting, right? [19:48:33] it might need some handling in ExtensionProcessor to handle paths, like we do with ResourceModules [19:48:46] it would be top-level, yes [19:49:31] I see in the v2 schema file that TrackingCategories and DefaultUserOptions are both top-level. I guess that can be confusing to some extent. On the other hand, I guess it's an implementation detail no-one but the consumer of the values need to worry about. [19:49:48] so +1 for keepin' it simple :) [19:50:20] my main intention was that we could transition things from globals into attributes without needing any changes on the extension side [19:50:39] Yeah, sounds great [19:51:06] legoktm: I'll need multi-layer merging, that is ResourceLoaderTestModule => qunit => modules[] [19:51:17] Anyway, will ping you if I'm stuck. [19:53:38] :) [19:54:15] I think there was an extension that automatically generated the list of test modules based on file existence in tests/qunit and file name [19:54:52] dunno if that's too magical, but I tend to like that kind of autowiring [19:55:23] Yeah, I don't like it for qunit because there's order and dependencies involved. [19:55:42] making that declarative isn't a net-win imho, it just makes maintenance harder. [19:55:54] works great for PHPUnit though [19:55:59] and selenium as well [19:56:31] in RL 3.0, I suppose deps could be localised per module directory and scooped up that way, for both prod and tests. [19:56:35] But that's for later. [20:12:31] legoktm: Hm.. so, I think remoteExtPath shouldn't exist. It requires the extension author to hardcode where the extension was installed. Afaik, looking at ExtensionRegistry code, it has that perfect knowledge already, as $dir. Sc we could change ResourceFileModulePaths from an object with localBasePath/remoteExtPath to just a string (like localBasePath), and even with a default of "", where remoteExtPath is just dirname + localBasePath. It [20:12:32] seems we already don't support silly things like ../ or /abs there, which is good. [20:18:22] Krinkle: it shouldn't exist in extension.json or in RL? [20:18:32] legoktm: extension.json at least [20:18:53] in RL for standalone-ness/separation of concerns. [20:19:16] localBasePath = $dir + relative. remoteExtPath = dirname + relative. [20:19:20] I'm coding this up [20:19:22] but.. [20:19:28] remoteExtPath / remoteSkinPath is biting me [20:19:34] ok, the one thing I note that right now is that ExtensionProcessor has no indication on whether it's an extension or skin [20:19:36] heh, yeah [20:19:51] OK. I'll shelve this for now [20:20:00] file a task at least? [20:20:20] The duplication of those two properties (having to set both localBasePath and extPath) is killing me. It's so fragile, and especially because extPath only affects debug mode for the most part [20:20:25] tbh the reason there's no way to differentiate is because it has never been needed [21:56:54] Krinkle: the other q is should we continue to conceptually support something other that qunit there, or can we simplify to just one level (ResourceLoaderTestModules: 'ext.ve.test'? [21:57:29] James_F: Yeah, I suppose we could make the attribute like ResourceLoaderQUnitTests [21:57:37] And remove that indirection from the perspective of extension.json [21:58:17] Unless we're planning to support frameworks other than QUnit? [21:58:47] No plans, but if we remove the separation it creates a tricky situation because these aren't plain module registrations [21:59:05] Ever wondered how they load on Special:JavaScriptTest? [21:59:07] Oh, sure, calling it out explicitly in the name as QUnit is what I was thinking. [21:59:46] it effectively cals OutputPage->addModules( array_keys( $testModules['qunit'] ) ) [22:00:04] So unless we introduce a separate thing for loading the modules, that indirection keeps it tidy. [22:00:43] so I'd be +1 for removing the indirection, but -1 on removing it from the attribute name, to avoid hurdles in the future or hard-to-maintain assumptions. [22:01:17] Let's do it. (Also the VE patch failed again.) [22:03:01] James_F: Ah yeah, I see why now. [22:03:13] I haven't done the expansion of json relative paths to real paths [22:03:21] e.g. __DIR__