Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Custom Rules Error #6811

Merged
merged 3 commits into from
Oct 28, 2020
Merged

Fix Custom Rules Error #6811

merged 3 commits into from
Oct 28, 2020

Conversation

PartMan7
Copy link
Collaborator

@PartMan7 PartMan7 commented Jun 7, 2020

/challenge [Gen 8] OU @@@ +Reshiram failed earlier, since the rule wasn't trimmed.

Also going to add the fix for chat only including the first rule.

@PartMan7
Fix Custom Rules Error
c5ab7fe 
`/challenge [Gen 8] OU @@@ +Reshiram` failed earlier, since the rule wasn't trimmed.

Also going to add the fix for chat only including the first rule.
@PartMan7
Copy link
Collaborator Author

PartMan7 commented Jun 7, 2020 •
edited
Loading

Okay, so the issue is that this.splitTarget disregards all arguments past the second comma, since the split cap returns ['user', 'format @@@ firstRule'] instead of ['user', 'format @@@ firstRule, nextRule, extraRuleForNoReason']. I can try editing it to use all rules, but this.splitTarget is a widely used function - is there anything that changing it might break?
If that sounds too risky, I could always just manually do it in the function itself.

Clarification: This is about using /challenge user, format @@@ rule#1, rule#2....

@PartMan7 PartMan7 marked this pull request as ready for review June 7, 2020 10:20
Zarel
Zarel reviewed Jun 7, 2020
View reviewed changes
sim/dex.ts Outdated
@@ -581,7 +581,7 @@ export class ModdedDex {
if (!customRulesString) return format.id;
const ruleTable = this.getRuleTable(format);
const customRules = customRulesString.split(',').map(rule => {
const ruleSpec = this.validateRule(rule);
const ruleSpec = this.validateRule(rule.trim());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be inside validateRule?

@Zarel
Copy link
Member

Zarel commented Jun 7, 2020

We should probably split by @@@ before using splitTarget.

@Zarel
Copy link
Member

Zarel commented Jun 7, 2020

Also, splitTarget shouldn't matter as long as the first , is guaranteed to be before the first @@@, because splitTarget only splits on the first , instead of all of them.

@Zarel
Copy link
Member

Zarel commented Jun 7, 2020 •
edited
Loading

I've been wanting to fix the multiple-rule issue but haven't had time, so thanks for looking into it, by the way!

@PartMan7
Copy link
Collaborator Author

PartMan7 commented Jun 7, 2020

Oh, didn't notice the further comments. I'll get on them.

@PartMan7 PartMan7 marked this pull request as draft June 7, 2020 15:01
Zarel
Zarel reviewed Jun 7, 2020
View reviewed changes
sim/dex.ts Outdated Show resolved Hide resolved
@PartMan7 @Zarel
Update sim/dex.ts
db37c93 
Right, sorry.

Co-authored-by: Guangcong Luo <guangcongluo@gmail.com>
@Zarel
Copy link
Member

Zarel commented Jun 7, 2020

Apparently this entire thing is kind of convoluted. I might need to take a look myself to figure out exactly which part is missing trimming.

@PartMan7
Copy link
Collaborator Author

PartMan7 commented Jun 7, 2020

When you say part, which are you talking about? I thought the rules were the only things missing trimming. 'o.o

@Zarel
Copy link
Member

Zarel commented Jun 7, 2020

The validation code is pretty complicated, and I would need to look at every level to figure out which step the trimming should be happening in.

@Zarel
Copy link
Member

Zarel commented Jun 7, 2020 •
edited
Loading

It's plausible to me that validateRule should simply throw an error if the input is untrimmed; the code that calls it looks like it would bug out with untrimmed input anyway.

@KrisXV
Copy link
Member

KrisXV commented Aug 8, 2020

I would like to bump this because this issue is still affecting /chall

@KrisXV
Copy link
Member

KrisXV commented Oct 8, 2020

bumping again @PartMan7 @Zarel

@PartMan7
Copy link
Collaborator Author

PartMan7 commented Oct 8, 2020

I thought Zarel wanted to suggest some changes; should I mark this as ready?

@PartMan7
Copy link
Collaborator Author

@Zarel Hiya?

@Zarel
Copy link
Member

Zarel commented Oct 20, 2020

Oh sorry I'll prioritize this.

@PartMan7
Copy link
Collaborator Author

At this point, I'm setting it as Ready for Review since there's been no comments at all.

@PartMan7 PartMan7 marked this pull request as ready for review October 23, 2020 04:26
@Zarel
Copy link
Member

Zarel commented Oct 23, 2020

I'm still working on this, although I sympathize with your impatience. Rest assured this is high priority to me.

@Zarel Zarel merged commit 062e424 into smogon:master Oct 28, 2020
@Zarel
Copy link
Member

Zarel commented Oct 28, 2020

Okay, this is definitely a good step. But I'm still confused about the splitTarget issue?

@Zarel
Copy link
Member

Zarel commented Oct 28, 2020

I tested /challenge [Gen 8] OU @@@ +Reshiram, +Zekrom and it seems to be working?

Zarel added a commit that referenced this pull request Oct 28, 2020
@Zarel
Improve custom rule validation
3b4a3ba 
Stray spaces are now consistently allowed in `/tour rules` and
`/challenge`, but are not allowed in `formats.ts`.

Improves #6811
@PartMan7
Copy link
Collaborator Author

When I tried it earlier, the challenge code you used wouldn't allow Zekrom. The first one would work as normal.

hsahovic added a commit to hsahovic/Pokemon-Showdown that referenced this pull request Nov 1, 2020
@hsahovic @KrisXV
@DaWoblefet @mia-pi-git @Zarel @iscke @TheImmortal @HoeenCoder @AnnikaCodes @Marty-D @sirDonovan @LegoFigure11 @urkerab @PlagueVonKarma @ACakeWearingAHat @VineST @KamilaBorowska @PartMan7 @edcrfv0 @DarkPhoenix911 @monsanto @Distrib-ps
Smogon master (#7)
7a529fd 
* Gigantamax Melmetal is unreleased

* Unskip passing validator test

* Remove /roomwhitelist from /roomhelp (smogon#7547)

* CommandContext: Make requireRoom support specifying a room (smogon#7549)

* Improve Chat.toDurationString

- Default precision is now 3.

  (Instead of "3 months 25 days 17 hours 46 minutes 40 seconds", it'll
	say "3 months 25 days 17 hours". You can still set the precision to
	`Infinity` if you actually prefer that.)

- Now displays "forever" for Infinity or overflow durations

- No longer skips blank precision levels

  (Shows "3 hours 0 minutes 10 seconds" instead of
	"3 hours  10 seconds")

* Mafia: Use new requireRoom API (smogon#7552)

* Youtube: Persist interval time (smogon#7550)

* Use new requireRoom API in trivia and scavs (smogon#7553)

* Publish 0.11.3 to npm

PS's sim engine is now available on npm!

sim/README.md describes how to use it:

https://github.com/smogon/pokemon-showdown/blob/master/sim/README.md

* Fix typo in sim/README.md

* Update OM banlists

* Random Battle improvements

* Validator: Reject sets with the Gigantamax flag that cant Gmax

* Gen 7: Random Battle updates

* Fix bug in toDurationString precision

Fixes smogon#7554

* Support replay URLs in /join (smogon#7526)

* Remove Battle of Legends

* Add Pikachu-Alola event

* Add legendaries to 1v1 & 2v2 banlists

* Remove Battle Stadium Series 6

* Consolidate Neutralizing Gas tests; add Gluttony/Slow Start NGas tests

* Clarify Neutralizing Gas + Imposter test

* Moderation: Improve modlog entry for /rangeban

* Validate timeout durations (smogon#7556)

* Add Crown Tundra Pokemon

TODO: Learnsets

* Add VGC 2020 mod

* Add VGC 2021 format

* Update hidden ability legality

* Missed this file in 42e918a

* Add new moves

* Add move descriptions

* Remove Dynamax Ubers Clause from Ubers

* Fix oversights from adding Crown Tundra DLC (smogon#7559)

* Add activation messages for Curious Medicine

credits marty

* Prevent Mega Rayquaza in gen 8

* Formats: Update banlists

* Let Rayquaza Mega Evolve in National Dex formats

* Crown Tundra: Add and Update Learnsets

It was mentioned that theres some lingering issues that existed
before this update with some pokemon not having Steel Beam/Draco Meteor
that should. I'm noting this here so its not forgotten.

* Update National Dex

* Fix typo in ability text

* Fix another typo

* Monotype: Unban Calyrex-Ice and Calyrex-Shadow

* Abilities: Fix Grim Neigh typo (smogon#7561)

* Fix Thunderous Kick

* Crown Tundra Learnsets: Add Missing Moves

My script had a flaw where it would not add entires for moves that
a pokemon did not learn previously (eg: Electric Terrain Tapu Koko,
Mystical Fire Latios, Close Combat Blaziken). This fixes this issue.

* Fix Moltres's stats (smogon#7562)

Apparently they got overwritten with galars counterpart

* National Dex: Ban Calyrex-Ice and Calyrex-Shadow

* Camomons: Ban Power Construct

* Fix Steel Beam error in learnsets

* Update 2v2 Doubles

* Fix As One attribution text

* Learnsets: Add new event data

* Validator: Allow Hidden Ability Entei/Raikou/Suicune in Gen 8 natively (smogon#7563)

* Add new aliases

* 2v2: Unban Kyurem-Black

* Prevent As One from being copied or removed

* Fix Eerie Spell

* Update obtainable items

Haven't seen confirmation of Soul Dew yet.

* Release the berries

* Show Genesect forme in Team Preview

* Implement As One correctly

* Add new Random Battle sets

* STABmons: Ban Lovely Kiss

* CAP: Unban Clefable

https://www.smogon.com/forums/posts/8635680/

* Fix crash in Punishments#autolock

We should be destroying a user's personal rooms _after_ making a modlog entry.

* As One cannot be suppressed, acquired, or removed

* Soul Dew is available

* Add Anything Goes ladder

* Add As One entry message

* Fix Dragon Ascent Rayquaza validation

* Monotype: Ban Calyrex-Shadow

* Update learnsets

Added legendary dynamax events.
Removed multiple level up entries.
Copy corrections to VGC20 mod.

* 1v1: Ban Magearna and Marshadow

* Convert VGC 2020's mod to a pre-DLC2 mod (smogon#7564)

* Add temporary format for DLC 1 metagame

* Add thread for VGC 2021

* Add Regigigas and Poipole events

* Monotype: Ban Zygarde

* Add Tundra Tourney (smogon#7565)

* Move Battlesearch to its own file (smogon#7481)

* DOU: Ban Jirachi, Melmetal, Dark Urshifu, Swagger

https://www.smogon.com/forums/threads/np-ss-dou-stage-5-one-more-time-jirachi-melmetal-urshifu-single-strike-swagger-quickbanned.3672010/post-8636825

* Update metagame threads

* Update Inheritance bans

* Add The Studio chat plugin (smogon#7542)

* Improve sim readme

* Clean up imports

A lot of our code for child processes doesn't really follow our
original standards. This refactors them back to work the way they
were intended to.

* Micle Berry is obtainable

* Add event Porygon

* Prevent Abilities from copying As One as well

* Battlesearch: fix crash in child processes

* Restrict Calyrex-Ice and Calyrex-Shadow correctly

* Fix Crown Tundra Pokedex validation (smogon#7573)

* Update descriptions

* NatDex Monotype: Ban Calyrex formes and Dragapult

* Tools: Modernize modlog entries for blacklists

* Calculate natures with 16-bit truncation (smogon#7540)

Also fixes Let's Go! which wanted to override these but couldn't.

* Fix typo

* Ignore As One boosts when already maxed out (smogon#7574)

* NFE: Ban Sneasel

* Add gift Toxel event data

* The Studio: Fix crash

* The Studio: Allow finding recs by artist/user name (smogon#7578)

* Monotype: Ban Calyrex-Ice and Genesect

* Ban Japanese Gen 1 Events in Int Formats: Part 2 (smogon#7581)


Co-authored-by: Guangcong Luo <guangcongluo@gmail.com>

* Ban Gen 1 Japan-only event moves

Fixes smogon#7516

* Update Lunar Dance to Gen 8 mechanics (smogon#7576)

* Random Doubles Updates (smogon#7569)

* Random Battle updates

* Rulesets: Fix crash in CCAPM2 (smogon#7582)

* Allow Randdubs Power Construct Zydog (smogon#7583)

* Add Overflow Stat Clause

* Camomons: Ban Dragonite & Kartana

* Move server/'s global variables out of index.ts (smogon#7585)

* AAA: Ban Kartana & Pheromosa

* Connections: Track current chat page (smogon#7522)

* Fix behavior of /faq with no arguments

* Studio: Handle Net errors better (smogon#7586)

* 1v1: Ban Dragonite

* Metronome Battle: Update ability banlist (smogon#7587)

* Youtube: Properly check permissions (smogon#7590)

* Chatlog: Use new username html tag (smogon#7519)

* Mix and Mega: Restrict Zygarde-C

* Add client text data

This syncs data/text to exactly match the client's data/text, in
preparation for them to be merged. The server's data/text will be
canonical, moving forward:

Relevant changes:

- A new file `data/text/default` has been added, for battle messages
  not associated with a move/item/ability.

- As One (Glastrier) and As One (Spectrier) should not have a start
  message; only As One itself.

- Hidden Power's `realMove` property does not belong in data/text
  and has been removed (it's still in data/moves where it belongs).

* Random Battle improvements

* /show: Throttle requests (smogon#7589)

* Make Eerie Spell's secondary effect secondary (smogon#7575)

* Youtube: Cache requested video data (smogon#7593)

* Indicate pending promises in /eval (smogon#7592)

* Remove sim/global-variables.d.ts

The sim no longer uses any global variables, so this can and should be
removed.

`server/global-variables` has also been cleaned up, since I'm working
on this.

* National Dex BH: Ban Electrify

https://www.smogon.com/forums/threads/national-dex-bh.3658587/page-10#post-8640772

* ZU: Ban Combusken

https://www.smogon.com/forums/threads/np-zu-stage-2-turn-it-again-combusken-quick-banned-77.3668115/page-4#post-8640782

* Remove last vestiges of tslint

* Fix Custom Rules Error (smogon#6811)

`/challenge [Gen 8] OU @@@ +Reshiram` failed earlier, since the rule wasn't trimmed.

* Improve custom rule validation

Stray spaces are now consistently allowed in `/tour rules` and
`/challenge`, but are not allowed in `formats.ts`.

Improves smogon#6811

* Make /clearstatus log to Staff room (smogon#7557)

`/forcerename` and other global punishments do this

* sendReplyBox: fix interaction with hidelines (smogon#7577)

* Chatfilters: Fix reasons displaying as undefined (smogon#7579)

* CommandContext: Add a method for getting roomgames (smogon#7531)

* Helptickets: Add button for the reporter's modlog (smogon#7560)

As per (this suggestion)[https://www.smogon.com/forums/threads/staff-suggestions-bugs.3514540/page-17#post-8634198].

* Random Doubles: Fix Regieleki

* Hosts: Support displaying shared IPs (smogon#7568)

* Punishments: Make rangelocks persistent (smogon#7545)

This solves the issue with yearlocks disappearing.

* Fix Slow Start's interaction with Neutralizing Gas (smogon#7580)

* Fix G-Max moves not showing up in movesearch (smogon#7555)

Happened because G-Max moves had move.isNonstandard as 'Gigantamax'.

* Support banning users from using groupchats (smogon#7558)

* Move Genesect and Naganadel to Uber

* Genesect formes should also be Uber

* Update National Dex UU threads

* M&M: Update bans and restrictions

* Chatlog: Carry opts over when switching days (smogon#7584)

* Fix G-Max Depletion (smogon#7571)

* Chat plugins: Make viewing source respect /permissions

* Fix Neutralizing Gas interactions

* Include groupchat bans in /punishments

* Improve error for Japan-only event moves

Thanks Plague Von Karma for help with wording!

Refs smogon#7581

* Reorganize README

The README has been split up so guides for PS's different use-cases are
easier to find.

* Chatlog: Allow non-trusted secret room ROs to view logs (smogon#7597)

* Ticketbans: Use the user object (smogon#7596)

* Improve documentation

Fixes smogon#7602

Also adds some other stuff, perhaps most notably why we don't use
package-lock so you don't have to dig for 2e85de3

* Studio: Only request YT video data once (smogon#7595)

* Add Doubles DLC 1 format

* Move natures into their own file (smogon#7601)

* BH: Add Dynamax Clause

* Update devDependencies

The newest `@types/node` fixes the `assert.strict.equal` deprecation
issue, so I'm updating more frequently than usual.

* Validate Natures in tests

(There's no need to validate natures somewhere that has an actual
performance impact.)

* Fix Hold Hands' num (smogon#7603)

* Add Galarica Wreath

* ANOTD: Change tagline to quote

Requested by Anime and Manga staff

* Add missing Trainer avatars

* Trivia: Document /trivia move

* National Dex Monotype: Unban Arena Trap

* Moderation: Actually make groupchatbans hotpatchable

* Moderation: Remove whitespace

* Helptickets: Properly quote and escape button HTML

* Add Berserk to the Berserk sim test

* Monotype: Update bans

https://www.smogon.com/forums/threads/ss-monotype-metagame-discussion-crown-tundra.3672167/page-3#post-8642660

* Fix Overflow Stat Clause and turn it into a mod (smogon#7604)

* Fix Ripen

* Users: Don't merge unregistered names across IPs

Fixes smogon#7594

* Don't report multiple punishments for a single room (smogon#7606)

* Add Pikachu-World event

* Add event Milcery

* National Dex Monotype: New bans and unbans (smogon#7608)

* AAA: Ban Dragapult, Dragonite, and Magearna

* Inheritance: Ban Regieleki and restrict Butterfree

* Remove VGC 2020 ladder

* Fix permission checks for global promotions

* Rename DLC 1 formats

* Modlog: Support logging to a SQLite database (smogon#7513)

* Modlog: Support logging to a SQLite database

Co-authored-by: Christopher Monsanto <chris@monsan.to>

* Modlog: Fix crash in writing global modlog

* Fix API for writing to the global modlog

* Trivia: Prevent stalemates in Infinite mode

* Update OU Sample Teams link

* Display the announcement whith /announcement (smogon#7612)

* Don't allow forcerenamed usernames to be reused (smogon#7609)

* Random Battle: Minor updates

* DOU DLC1: Fix banlist

* Improve and number design standards

* Modlog: Disallow duplicates in ModlogEntry#alts

* DOU: Ban Marshadow, Shadow Tag

https://www.smogon.com/forums/threads/np-ss-dou-stage-5-one-more-time-jirachi-melmetal-urshifu-single-strike-swagger-quickbanned-marshadow-shadow-tag-quickbanned.3672010/post-8644210

* Build: Update dependency check

* Trademarked: Update bans

* Fix Streams bug

It turns out 001f98b was wrong.

When urkerab asked why it `peek` wasn't awaited:

smogon@e91c4c5#commitcomment-41364837

The answer was because clearing the buffer after peeking needed to
happen synchronous: if the buffer is written to after peeking but
before the buffer is cleared, that write is lost forever.

This just goes to show, if you do something subtle enough to require
type assertions, you should probably add a comment about what's going
on.

Fixes smogon#7605

This also removes `BattleStream#start()` which is completely useless
API complication. A better implementation would properly forward
crashes between streams (maybe `pipeTo` should do this) but as it
stands, it's not doing anything.

* Improve "click here" error message

* Add November's OMotM, LCotM and RoA Spotlight (smogon#7615)

* NFE: Ban Haunter and Pawniard

* Update Doubles tiers

* CAP: Fix Mega Crucibelle being allowed

* Fix [Gen 3] Doubles OU

* Scalemons: Add Overflow Stat Mod

* November tier update

* Update NU-legal Pokemon

* Update National Dex tiers

* CODEOWNERS: Add chaos to anything SQLite related

* Config: Move Bot rank below Driver  (smogon#7617)

* Revamp Gen 2 Random Battle (smogon#7610)

* Fix /randbats interaction with Gen 2 sets

* Remove unused definitions

* Modlog: Avoid optional properties in ModlogEntry  (smogon#7613)

* Modlog: Avoid optional properties in ModlogEntry

* Use a type

* Make IP nullable

* Add NOT NULL constrains

* Fix typo

* Better handle races when reading Streams

* Mark other Genesect formes as unreleased in DLC1

* Gen 2 Random: Fix Rest + Sleep Talk

* Add new PU bans

* Gen 2 Random: Fix typo

* NU: Unban Drought

Also updated tier threads.

Co-authored-by: Kris Johnson <11083252+KrisXV@users.noreply.github.com>
Co-authored-by: Leonard Craft <leonardcraft64@gmail.com>
Co-authored-by: Mia <49593536+mia-pi-git@users.noreply.github.com>
Co-authored-by: Guangcong Luo <guangcongluo@gmail.com>
Co-authored-by: iscke <frumgerth@gmail.com>
Co-authored-by: The Immortal <the_immortal123@live.com>
Co-authored-by: HoeenHero <HoeenCoder@users.noreply.github.com>
Co-authored-by: Annika <56906084+AnnikaCodes@users.noreply.github.com>
Co-authored-by: Annika <hehe@doesntexist.com>
Co-authored-by: Instruct <66930476+xInstruct@users.noreply.github.com>
Co-authored-by: Marty-D <Marty-D@users.noreply.github.com>
Co-authored-by: Quinton Lee <quivistis@gmail.com>
Co-authored-by: LegoFigure11 <24732684+LegoFigure11@users.noreply.github.com>
Co-authored-by: urkerab <urkerab@users.noreply.github.com>
Co-authored-by: May Evans <36418502+PlagueVonKarma@users.noreply.github.com>
Co-authored-by: ACakeWearingAHat <45981036+ACakeWearingAHat@users.noreply.github.com>
Co-authored-by: VineST <alek.malesevic@yahoo.com>
Co-authored-by: Konrad Borowski <konrad@borowski.pw>
Co-authored-by: PartMan <47669599+PartMan7@users.noreply.github.com>
Co-authored-by: Charlie Kobayashi <sparkychildcharlie@gmail.com>
Co-authored-by: edcrfv <70171487+edcrfv0@users.noreply.github.com>
Co-authored-by: DarkPhoenix911 <64982718+DarkPhoenix911@users.noreply.github.com>
Co-authored-by: Christopher Monsanto <chris@monsan.to>
Co-authored-by: Distrib <theodelhay@orange.fr>
Co-authored-by: Annika <annika0uwu@gmail.com>
Quanyails pushed a commit to Quanyails/Pokemon-Showdown that referenced this pull request May 12, 2021
@PartMan7 @Quanyails
Fix Custom Rules Error (smogon#6811)
a17a9ce 
`/challenge [Gen 8] OU @@@ +Reshiram` failed earlier, since the rule wasn't trimmed.
Quanyails pushed a commit to Quanyails/Pokemon-Showdown that referenced this pull request May 12, 2021
@Zarel @Quanyails
Improve custom rule validation
049feda 
Stray spaces are now consistently allowed in `/tour rules` and
`/challenge`, but are not allowed in `formats.ts`.

Improves smogon#6811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zarel Zarel Zarel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@PartMan7 @Zarel @KrisXV