Remove MWCapabilities object (partial impl) #1878
Conversation
| /** | ||
| * veApiUrl based on top of 'new ApiURLDirecto' | ||
| */ | ||
| public get veapiUrl(): URL { |
There was a problem hiding this comment.
veApiUrl(), restApiUrl(), etc. are shortcuts and offer lazyness to the URL directors. I see little value to have them. I suggest to just something like this.apiUrlDirector.getVisualEditorURL()` which works in a lazy manner.
If you you think this.apiUrlDirector.getVisualEditorURL() is really too cumbersome, then OK to just keep the redirection part, but I see no reason why the lazyness is not built in the Director itself considering that the configuration of it happens at construction time.
| // TODO: This depend on baseUrlDirector.buildURL(this.apiPath) and looks like a weak solution | ||
| this._veapiUrl = this.apiUrlDirector.buildVisualEditorURL() | ||
| } | ||
| return this._veapiUrl |
There was a problem hiding this comment.
Not easy, considering that you have so many different styles and developers behind the codebase... but think about:
UrlvsURLVEvsVisualEditor.
There was a problem hiding this comment.
I'd like that we have a dedicated ticket about naming conventions.
There was a problem hiding this comment.
For VE, the answer is in the validation of https://github.com/openzim/mwoffliner/wiki/API-end%E2%80%90points. For URL, I clearly prefer to keep full uppercase for constants but up to you.
There was a problem hiding this comment.
I renamed ve to visualEditor, but regarding Url, I'd rather keep it as is because there are about ~580 entries across mwoffliner that use the same capitalization. Check here.
| action: 'query', | ||
| format: 'json', | ||
| // TODO: Do we need this.mwCapabilities.coordinatesAvailable here? | ||
| prop: `redirects|revisions${this.mwCapabilities.coordinatesAvailable ? '|coordinates' : ''}${this.getCategories ? '|categories' : ''}`, |
There was a problem hiding this comment.
Here there is a chicken-egg problem, not sure how this happened, but you clearly need to put |coordinates to then see if this is supported.
|
|
||
| this.modulePath = baseUrlDirector.buildModuleURL(config.modulePath) | ||
| // Default capabilities | ||
| // TODO: check whether to remove this object |
There was a problem hiding this comment.
It seems you are almost done and ready to get rid of this mwCapabilites.
| /* | ||
| TODO: Cookie is shared between Downloader and Mediawiki, probably antipattern. Use as interim solution for now. | ||
| Also, double-check possible race condition - cookies should be set before checking capabilities. | ||
| */ | ||
| downloader.loginCookie = resp.headers['set-cookie'].join(';') |
There was a problem hiding this comment.
Indee... here would help to clearly define what is the duty of the Mediawiki and Downloader class. Once this is clear, easier to refactor.
| @@ -296,6 +401,12 @@ class MediaWiki { | |||
|
|
|||
| return mwMetaData | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
A few comments have been lost here, maybe on purpose?! Just try to keep things commented when needed.
| @@ -253,3 +254,12 @@ export function mwRetToArticleDetail(obj: QueryMwRet): KVS<ArticleDetail> { | |||
| } | |||
| return ret | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Maybe a comment about what the purpose of this? At a first look, I can not remember... or is this totally new? Sounds a bit strange to have it here... but here again, not sure about the really purpose of mw-api.ts.
There was a problem hiding this comment.
I replaced checkApiAvailabilty() method from Downloader because it is more like a utility function.
|
@VadimKovalenkoSNF I had a first look and for the MWCapabilities, even if the work is not over, it looks good and coherent to me. I see a lots of code which is not related to the fix, not sure if this is not rebased properly on main, if you have other commits unrelated, .... |
|
This has been fixed in #1839 |
Fixes #1357