Opened 8 months ago

Closed 7 months ago

#7425 closed change (duplicate)

Remove specialization attribute from subscriptions.json

Reported by: greiner Assignee:
Priority: Unknown Milestone:
Module: Automation Keywords:
Cc: tlucas, sebastian Blocked By: #7091, #7371
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by greiner)

Background

We stopped accessing the "specialization" attribute in subscriptions.xml in #7091 and instead rely on our own mapping. In #7371 we converted that file to JSON and also removed unused attributes from it. Therefore we can also remove the "specialization" attribute from that JSON after #7091 has made it into the extension.

What to change

Remove "specialization" attribute from data/subscriptions.json

Change History (13)

comment:1 Changed 8 months ago by greiner

  • Description modified (diff)
  • Summary changed from Remove spezialization attribute from subscriptions.json to Remove specialization attribute from subscriptions.json

Fixed typo.

comment:2 Changed 8 months ago by sebastian

  • Cc sebastian added

I'm inclined to merge this into #7371. FWIW, we already removed author which is no longer used either, there as well.

comment:3 Changed 7 months ago by tlucas

Already removing this in #7371 would significantly decrease the script's complexity - i'm also in favor of doing this right away. @greiner, would you agree? Can we already do this?

comment:4 follow-up: Changed 7 months ago by greiner

As mentioned in the ticket description, we only stop depending on it after #7091 makes it into the extension which is not expected to happen anytime soon. Therefore we'd have to either rush it or extract the relevant changes from it and only rush those.

Neither way would be desirable for us since we already have two dependency updates lined up (see #7308 and #6936) but the latter approach would be feasible if you insist on it.

comment:5 in reply to: ↑ 4 Changed 7 months ago by tlucas

Replying to greiner:

As mentioned in the ticket description, we only stop depending on it after #7091 makes it into the extension which is not expected to happen anytime soon. Therefore we'd have to either rush it or extract the relevant changes from it and only rush those.

Neither way would be desirable for us since we already have two dependency updates lined up (see #7308 and #6936) but the latter approach would be feasible if you insist on it.

I don't insist on it and i'm also fine with generating specialization for the time being - FWIW the generation is already done and working, so no extra work on my side.

comment:6 follow-up: Changed 7 months ago by sebastian

You have to adapt for #7371 (parsing JSON instead of XML) in adblockplusui anyway. Would it be a big deal to stop using specialization while doing so (independent of #7091)? Where is that value shown in the UI anyway?

comment:7 in reply to: ↑ 6 Changed 7 months ago by greiner

Replying to sebastian:

You have to adapt for #7371 (parsing JSON instead of XML) in adblockplusui anyway. Would it be a big deal to stop using specialization while doing so (independent of #7091)?

We have already made that change in #7091 and depending on when the next major release is going to happen, we may be able to include it already in that one. It'd be great if we could avoid a separate dependency update for it though. You're right that we have to make adjustments to stay compatible after the introduction of package.json anyway and that the change could be included in the same dependency update as that one.

So one way or another we'll end up with redundant work because either Core or UI code needs to be removed again after #7091 lands.

But as I said, if you insist on it, we can make that change.

Where is that value shown in the UI anyway?

It's used to represent the various language filter lists in the Language section of the options page's General tab because we decided against translating language names.

comment:8 Changed 7 months ago by sebastian

Given #7091 is already merged, any reason to not include it in the dependency update that will include the changes adapting for #7371? I guess I'm not insisting but I fail to see the point of going to quite some length in order to preserve specialization with #7371, if it's matter to be removed (and the related changes already landed in some branch).

comment:9 Changed 7 months ago by greiner

#7091 is a major change that hasn't been through QA yet and the release branch it's in is not yet ready for dependency update. On top of that, we do have #6936 which is ready and therefore should be included first.

comment:10 Changed 7 months ago by greiner

After further discussing this question with Winsley and considering that not having translations for the newly added string is not a regression, I think we can provide the requested change as part of #7431.

comment:11 follow-up: Changed 7 months ago by sebastian

Will that be the same dependency update where you adapt for using subscriptions.json instead of subscriptions.xml?

BTW, #7091 is listed to be included in #6936.

comment:12 in reply to: ↑ 11 Changed 7 months ago by greiner

Replying to sebastian:

Will that be the same dependency update where you adapt for using subscriptions.json instead of subscriptions.xml?

The scope of compatibility releases is not determined by us but by Platform and Core changes since those releases only exist to stay compatible with both of them. But yes, that's how I'd go about it.

BTW, #7091 is listed to be included in #6936.

The font generation tool was first introduced in that UI release (i.e. release-2018-5) but we only started using it for the one after that (i.e. release-2019-1). Therefore it's worth noting but not relevant for #6936.

comment:13 Changed 7 months ago by sebastian

  • Resolution set to duplicate
  • Status changed from new to closed

Cool, so we can go ahead, and merge this into #7371 then.

Note: See TracTickets for help on using tickets.