Towards opinions in PolicyLists. #336
Conversation
This changeset is part of an ongoing effort to implement "opinions" within policy lists, as per MSC3847. For the time being: - we rename BanList into PolicyList; - we cleanup a little dead code; - we replace a few `string`s with `enum`; - `ListRule` becomes an abstract class with two concrete subclasses `ListRuleBan` and `ListRuleOpinion`.
| * The recommendation for this rule, or `null` if there is no recommendation or the recommendation is invalid. | ||
| * Recommendations are normalised to their stable types. | ||
| * A glob for `entity`. | ||
| */ | ||
| public get recommendation(): string|null { | ||
| if (RECOMMENDATION_BAN_TYPES.includes(this.action)) return RECOMMENDATION_BAN; | ||
| return null; |
There was a problem hiding this comment.
What's the reason for changing this getter? You need to be really careful to check where this behaviour is being relied upon https://github.com/matrix-org/mjolnir/pull/336/files#diff-ebc449200d5204df932d4794476267f12b9759b8dfcbf34b7dc92361870b5f3cR155 is the only example (I think) and needs changing to ensure the kind is upgraded to stable.
There was a problem hiding this comment.
Well, for ListRuleBan, the public readonly property will return exactly the same thing as the getter, with simpler code and slightly stronger typing.
For ListRuleOpinion, the public readonly property will return something different, but there is no helping it. I'll need to go through the entire codebase with a fine comb to find out exactly where the fact that all rules are m.ban is currently harcoded. I expect that this is going to take me some time, hence the current PR that only does part of the job.
There was a problem hiding this comment.
I'll need to go through the entire codebase with a fine comb to find out exactly where the fact that all rules are m.ban is currently harcoded. I expect that this is going to take me some time, hence the current PR that only does part of the job.
Well yes but you can't do part of the job without introducing a bug so it doesn't seem acceptable, though searching this myself there is only one instance (the one linked) where there would be a bug.
There was a problem hiding this comment.
Bugs happen, but I believe that I've structured my refactoring to ensure that the behavior remains unchanged for the time being. There may be slight theoretical differences in ill-formed rules if someone happens to create an ill-formed ListRuleOpinion, but that shouldn't have any consequence.
Do we agree so far?
There was a problem hiding this comment.
I'm not sure that we're talking about the same thing, but so long as we only make rules with List.parse i don't think this will be a problem anymore.
package.json
Outdated
| @@ -14,6 +14,7 @@ | |||
| "start:dev": "yarn build && node --async-stack-traces lib/index.js", | |||
| "test": "ts-mocha --project ./tsconfig.json test/commands/**/*.ts", | |||
| "test:integration": "NODE_ENV=harness ts-mocha --async-stack-traces --require test/integration/fixtures.ts --timeout 300000 --project ./tsconfig.json \"test/integration/**/*Test.ts\"", | |||
| "test:YORIC": "NODE_ENV=harness ts-mocha --async-stack-traces --require test/integration/fixtures.ts --timeout 300000 --project ./tsconfig.json \"test/integration/banListTest.ts\"", | |||
There was a problem hiding this comment.
don't forget to remove this 😸
| const banList = new BanList(banListId, banListId, mjolnir); | ||
| await createPolicyRule(mjolnir, banListId, RULE_SERVER, 'exmaple.org', '', {recommendation: 'something that is not m.ban'}); | ||
| const banList = new PolicyList(banListId, banListId, mjolnir); | ||
| await createPolicyRule(mjolnir, banListId, RULE_SERVER, 'exmaple.org', '', { recommendation: 'something that is not m.ban' }); |
There was a problem hiding this comment.
Could you explain how these formatting changes were spotted? (Did you use another linter or editor extension?) Dealing with a merge conflict in this file and worried about missing some.
This changeset is part of an ongoing effort to implement "opinions"
within policy lists, as per MSC3847.
For the time being:
strings withenum;ListRulebecomes an abstract class with two concrete subclassesListRuleBanandListRuleOpinion.