Add LGTM code quality badges #4843
Conversation
|
Hey @AustinXII Can you please point me to the warnings you suspect to be false positives? If you find it cool, our recommended usage is intergation with Pull request, so that LGTM reports any new problem (or fix) directly within the Pull Request. It's a free one-click operation |
|
I see. |
|
@xcorail Please ignore Austin, he doesn't speak for us. @AustinXII I'm very confused how useless code is supposed to "prevent issues for other users on their servers". :| |
|
@xcorail I think this is really cool! Some false positives include:
Which is code designed to shut TypeScript up. I don't actually know how to define a property to TypeScript without changing its existing value. It looks like my options are to suppress it or report it to you guys. I think you guys might benefit from ignoring this error specifically when it's of the form |
|
The other issue is:
This is in an example code file, and it already has There are another few instances in which Everything else LGTM has discovered appears to be a real code quality issue, though. This is really cool, thanks for bringing this to my attention! |
|
Yeah I didn’t read all the errors at the time and made a bad false assumption on what would bring an alert considering the amount of errors, mb. I thought it would also be giving errors for lines like |
|
@xcorail Also, I pushed a commit to Pokemon-Showdown-Client five hours ago: smogon/pokemon-showdown-client@a133ee8 And it doesn't seem to be reflected in: https://lgtm.com/projects/g/Zarel/Pokemon-Showdown-Client/ Is there some way to manually trigger a re-analysis? Also how often should I expect it to update if I don't? |
|
Hello @Zarel Thanks for stepping in, I was also a bit confused :-) So
|
|
I found another really interesting false positive here: https://discuss.lgtm.com/t/false-positive-this-expression-always-evaluates-to-true/1270/1 |
|
I'm not sure I like that comments are posted by my own account, like in: #4845 (comment) TravisBuddy uses its own GitHub account, which I think makes things much clearer. |
|
Hi @Zarel You are absolutely right. On BitBucket, our comments are posted by LGTM, not by your personal account. And for the other false positive, our engineers are on it, they suspect a bug on our end (so thanks for reporting it). |
|
Semmle JavaScript engnineer here. Thank you for the feedback!
A fix for this is about to be merged, it should be online in about two weeks time (github/codeql#166).
We already support some suppression comments from linters, I will add this to the list of cases to consider. I do not have an estimate for when it will be online. |
|
Hey there Just wanted to let you know that during 6 weeks, for each LGTM alert fixed, we'll make a donation to WWF. You can also win a free ticket to GitHub Universe, with travel and accommodation. Details here: https://competitions.lgtm.com/ghu-2018 |
|
My friend is having trouble suppressing these alerts: |
|
Okay, I just fixed all the LGTM issues across the three PS repositories. There are 11 recommendations left which I can't fix:
|
Excellent.
Thank you for the feedback. |
|
@esben-semmle Thanks! No need to deal with the |
|
(from #4962) This is concerning. This project uses It looks like |
|
Thank you for raising that issue, we will look into it. |
|
Thank you once more for reporting this. The PR alert view only shows relevant alerts now. |
c8b321b to
d90ac63
Compare
|
Unless I'm mistaken all lgtm warnings should be fixed with #5139 |
|
Well done! I agree that the eight remaining alerts are not something that you need to fix, but I can not really see a satisfactory way to avoid flagging those locations without just whitelisting the locations manually. |
|
@esben-semmle, is there a way to whitelist a location on another line? I've been trying to enforce line length limits. Something similar to |
No, not at the moment. Sorry. |
README.md
Outdated
| @@ -10,6 +10,8 @@ Navigation: [Website][1] | **Server repository** | [Client repository][2] | [Dex | |||
| [](https://travis-ci.com/Zarel/Pokemon-Showdown) | |||
| [](https://david-dm.org/zarel/Pokemon-Showdown) | |||
| [](https://david-dm.org/zarel/Pokemon-Showdown?type=dev) | |||
| [](https://lgtm.com/projects/g/Zarel/Pokemon-Showdown/context:javascript) | |||
There was a problem hiding this comment.
| [](https://lgtm.com/projects/g/Zarel/Pokemon-Showdown/context:javascript) |
Okay, I'll take the alert count, but the code quality badge seems redundant/vanity.
This comment has been minimized.
This comment has been minimized.
|
I got the error:
But there's no write there; it's purely a declaration. |
|
Thanks, we are aware of that problem. A fix should be live in a weeks time. In the meantime, I have manually disabled that query for your project. |
* Gen 1: Paralysis should not reapply stat drop because of a failed move (smogon#5123) * Random Battle: Fix Dedenne HP EVs (smogon#5122) * Mafia Theme Update (smogon#5114) * SSB: Fix Compost & Wonder trade with False (smogon#5124) * Support custom rated messages (For use in SPL) * Fix dc6fc43 * Scavs: Award points for hosting automatically (smogon#5126) * Mafia: Bugfixes for modifiers, allow searching for terms (smogon#5127) * Better fix for dc6fc43 * Battle Factory: Split UU Hydreigon sets * Let's Go OU: Remove suspect notice * PU: Ban Alolan Exeggutor * Fix crash, message for locks (smogon#5125) * Datacenter: Add more ranges (smogon#5129) * Battle Factory: Fix another Hydreigon set * NU: Add Vanilluxe suspect notice * Add ORAS 1v1 and DPP AG * Update Trick Room's short description * Add 2nd and 3rd place point information to trivia modlog * Remove a comment in Trivia code about point values They no longer need to be divisible by 5. The caps are unlikely to change, however the technical limitation of values having to be divisible by 5 no longer exists. * Notify after, not before, deleting a help ticket (smogon#5130) * Update CAP analysis links * Support new avatars This isn't full support, but it's the important part, the part that lets you use past-gen avatars. * Other Metagames of the Month February 2019 (smogon#5131) * Add February RoA Spotlight * Fix 'sage-gen2jp' avatar * Update allowed avatars list * Remove Pokebilities format I'm no longer accepting non-TypeScripted formats. * Fix missing comma * Remove useless param from pokemon.transformInto Introduced in 743c851 * Stadium: Fix Counter and probably other things * Remove Simple Symphony * Always display Alola formes in /ds (smogon#5133) The Alola formes are so different to regular formes that displaying both the regular form and Alola form is actually a useful information. * Fix some OMotM crashes (smogon#5132) * Deprecate battleban * Fix unnecessary ts-ignore * Pokebilities: Ban Moody (smogon#5136) * Update Me First * Spotlights: Allow non-staff to see current spotlights * Spotlights: allow multiline input * Remove useless check from pokemon.transformInto * Remove useless check * Make it harder to timerstall This commit is aimed at a particular kind of timerstalling: starting a new game while letting the timer run out on your current game, instead of just forfeiting. Now you can't search for more battles if it's your turn to move, or if your opponent takes less than 10 seconds to move (including animation). 1v1 and Metronome Battles asked for exemptions, and they've received them. * Pikachu-Alola is not an Alola form for dex search (smogon#5138) The intent of a recent dexsearch change was to display Alola formes separately as they are very different. However, this also meant showing Pikachu-Alola which is not an Alola form despite having "Alola" as a forme name. * Gen II: Fix species items * Reorder formats * Formats: Fix typo and add more thread links * Update command documentation (smogon#5128) * Move server code to server/ Also move mods/ to data/mods/ This makes PS more monorepo-like. The intent is to further separate the sim and the server code, but without fully committing to splitting the repository itself. We now support `./pokemon-showdown start` in addition to `./pokemon-showdown`. I'm not clear which I want to be the default yet. * Fix formatText support for `&` `>` etc in URLs * Fix StaticServer and loading rooms * Revert url for chatrooms.json updates * Fix Illusion when dragged out * TypeScript globals A few globals: Monitor, LoginServer, etc weren't being correctly TypeScripted. This should change that. * Datacenters: Add a range (smogon#5141) * Fix Hotpatch * Fix the rest of the hotpatches * Add BH suspect test * Rename channel -> room, subchannel -> channel "channel" is just a fancy way of saying "room, but in sockets". Renaming them like this should make it clearer exactly what's going on in sockets. * Fix some LGTM alerts (smogon#5139) * Pokebilities: fix special abilities as well as Gastro Acid (smogon#5140) * STABmons: Ban Thundurus * RU: Ban Stakataka * Help tickets: Remove timerstalling * Battlesearch improve UI * LC: Ban Baton Pass * Check capitalization in Factory sets This might be debatably important, but if it's worth pointing it out in reviews, we might as well automate it. * Use `declare let` instead of `let` in globals.ts * Fix chat plugin file I/O after server/ refactor * Fix modlog search location * Battle Factory: Update RU and LC (smogon#5143) * Fix hotpatch uncaching Chat.uncache functions now consistently take a path relative to repo root, just like FS. Next step will probably be to have FS handle all text-based databases. * Fix crash in /data * Mafia: Support hiding discards, starting from open signups (smogon#5142) * Refactor Emoji regex to use \p syntax (smogon#5146) (Increases minimum Node version requirement) * Increase required version number in README * Suppress LGTM's URL sanitization warning (smogon#5147) This warning is strictly speaking correct, the URL isn't verified properly. However, in this case, this isn't a practical issue, as it's possible to use /forcebattleban command anyways. * Add Recoil as Move Search Parameter (smogon#5149) * Help Tickets: Require users to explain their issue before getting help (smogon#5148) * Fix ticket activation when the creator is staff * STABmons: Fix Thundurus ban (smogon#5150) Only the base forme was supposed to be banned. * Formats: Fix thread links * Movesearch: Account for custom recoil (smogon#5151) * README: Add notes on upgrading Node version This should help Linux users. * Add LGTM alert count badge (smogon#4843) * Allow free multi-laddering in first five turns Searching for multiple games in the first five turns is unlikely to be timerstalling, so we allow it here. * Improve choice error messages This should make choice errors clearer. * Fix crash in /processes * Better support IPv6 in IP checker * Fix minor typos * Fix Help Tickets preview * Add in-battle mod detection to /weak and /coverage (smogon#5144) * Clarify message for multi-laddering * Fix hotpatching battles/formats Hotpatching battles only is no longer possible - it takes the same amount of resources as hotpatching formats but is way more error-prone. * Remove 7 second limit for multi-laddering * Automatically end `BattleStream`s at end of battle PS prefers to keep battle streams open (for `/evalbattle`) until manually ended, but this is confusing some other users. We now automatically end streams unless the `keepAlive` option is set. Fixes smogon#5157 * Fix /hotpatch chat Old chat plugin processes weren't getting correctly ended. * Tweak ghost connection timeout, log their connection status * Fix an edgecase regarding locks getting removed on guests This caused /unnamelock to leave ghost users in the userlist * Fix math for counting battle turns Turns out, requestCount goes up by 2 every turn, not 1. In hindsight, this makes sense because we're giving every player their own request ID. * Random Battle: Update Probopass (smogon#5159) Earth Power is one of its most used moves because of Steel trapping with Magnet Pull, while Pain Split is almost unheard of in standard tiers, and isn't very useful on it unless it has Sturdy (but Magnet Pull is the better move and is rated higher) * Refactor formeChange This fixes an obscure issue where we sent a formechange-to-self on faint in certain obscure situations (apparently including Gen 4 Custap?) In case anyone wants to investigate the details (this commit just patches over some symptoms), the relevant replay is here: https://replay.pokemonshowdown.com/smogtours-gen4ou-425031 * Require object literal method shorthand * Deprecate /redir * NU: Ban Vanilluxe * Remove deprecated commands from /help * Random Battle improvements * Mafia: Fix starting games through the UI (smogon#5156) * Add a PageContext for HTML pages and expand html page functionality (smogon#5160) * Show deprecated status of commands in the command help * This command is not technically deprecated but close enough * Support the random battles damage calculator * Random Battle: Update Rhyperior (smogon#5162) * Random Battle: Update Rhyperior - Although Rock Blast is the preferred move in RU, there are notably fewer Sturdy mons in Random Battle and it struggles more with breaking Substitutes with it. - Aqua Tail gives very minimal meaningful coverage since it mostly is super effective on Pokemon already weak to Ground. Instead, give it Ice Punch since it's much more common on it. * Update formats-data.js * Update formats-data.js * DPP LC: Ban Hypnosis * Battle Factory: Remove unranked Pokemon and fix Snorlax (smogon#5164) * Revamp evolution tables (smogon#4997) * Add LC suspect notice (smogon#5169) https://www.smogon.com/forums/threads/lc-suspect-bush-doesnt-care.3647220/ * Change (PU) to Untiered (smogon#5170) * Prevent using Leppa Berry from triggering Endless Battle (smogon#5158) Previously, using Leppa Berry by itself caused Endless Battle Clause to consider Pokemon to be stale. However, it is reasonable to use Leppa Berry without an intent to cause an endless battle - for instance to increase the PP of a move with a low PP. * Fix transformInto so OMs still work (smogon#5165) * Add tests for Sleep Clause Mod (smogon#5171) * Cafe: Fix path * Fix terrain seed activation timing (smogon#5163) * CAP: Nerf Crucibelle-Mega (smogon#5172) * CAP: Nerf Crucibelle-Mega * oops * Revert "Fix terrain seed activation timing (smogon#5163)" (smogon#5173) This reverts commit 250955e. * Fix "Fix terrain seed activation timing (smogon#5163)" Shouldn't of reverted this so quickly. Credit to the code goes to @MacChaeger * Add alias for /hidetext * Add Bottle Caps * Mafia: Fix html room updates (smogon#5174) * Battle Factory: Fix Espeon's ability in RU (smogon#5175) * Fix crash in ghost connection fix * 1v1: Unban Kyurem-Black (smogon#5176) * Don't crash when using /data in with the camomons mod * TCG: Update wiki domain * Allow warning users outside the room in all global warn contexts * Fix Chimecho's egg group * Support custom hideReplay setting * Use editprivacy permission for /ionext This matches the permission for /ioo * Update .gitignore for /server/ * Optimize Dex#getEffect by caching the result. (smogon#5181) As outlined by pkmn.cc/optimize, this change should result in a ~30% increase in Battle performance over repeated runs. Caching the resultant effect is safe provided nothing mutates it. * SSB: Fix Evoblast interaction with substitute; update ranks (smogon#5166) * Learnsets: Fix Gen 4 tutor moves * Refactor Dex#getEffect + Dex#getEffectInner back to one method (smogon#5182) * Add evolution method information (smogon#5179) * Improve data representation of evo method * Simplify evo data representation * Add 1v1 suspect notice (smogon#5186) * Disallow 0 EVs even when EVs weren't sent (smogon#5187) * Refactor remaining tests to use Battle#makeChoices (smogon#5183) * Remove * from most permissions shown in /help (smogon#5168) * Tours: Fix a forfeit being reported as failed even though it succeeded (smogon#5188) * Finally remove deprecated legacy API It's gone! The last remnants of the old choice parsing system is finally gone! * Lower tab size in !code * Chat monitor: make inap pokemon names weeklock instead of namelock * Fix random typos in tests (smogon#5190) * Split `getRelevantEffectsInner` This is mainly a readability refactor. * Refactor getRelevantEffects -> findEventHandlers This fixes getRelevantEffects's massively confusing call structure. Most parameters relating to bubbling up and down have been removed entirely, and traversing the many possible event handler types is now simply done outside the callers. In addition, a lot of variables/functions have been renamed for much better clarity. * Random Battle: Improve Toucannon * Mafia: Resolve chat pages properly (smogon#5192) * Datacenters: Add a range * Refactor findEventHandlers further (smogon#5193) - rename `thing` when we know what it will be. - simplify `resolveLastPriority` - remove `findBattleEventHandlers` param given that its always this - avoid multiple lookups of the same attribute These functions related to finding events are very hot (pkmn.cc/optimize) and microbenchmarks indicate TurboFan is unable to extract out the constants for us. As a side benefit, there's less `// @ts-ignore` required, and future optimizations will use the constants anyway. More extraction can be done in findEventHandlers, but that will be left for a future change where the need becomes more obvious. * Mark Feebas's evolution in Gen4 as requiring high Beauty Trade evolution for Feebas was introduced in Gen5. * Remove 'server/process-manager.js' from tsconfig (smogon#5197) 'process-manager.js' now lives under 'lib/' which is already included. * Add 1v1 Sample Teams (smogon#5195) * Add 1v1 Sample Teams * Add missing comma * Fix some Typescript related TODOs (smogon#5198) * Datasearch: Fix event Pokemon showing in LC searches (smogon#5177) * Document remaining message types in SIM-PROTOCOL (smogon#5196) * Add Imprison switch-out test Apparently we had zero tests to make sure volatile statuses go away after switching out and back in! * Further improve switch tests - Test that Baton Pass passes Ingrain - Test that Baton Pass doesn't pass Imprison - Test that other forms of switching don't pass either * Update TypeScript to version 3.3.3333 Using a tilde in a version number as TypeScript explicitly doesn't follow semver according to microsoft/TypeScript#14116. * Pokebilities: Ban Porygon-Z (smogon#5200) * Refactor getFormat pattern and ordering for speed (smogon#5194) Even though 31c6a32 added caching to make this method less of a hotspot, there's no reason to use a pattern which is an order of magnitude slower (https://jsperf.com/pokemon-showdown-getEffect). Also, rearranges Format to get checked first as pkmn.cc/optimize finds it to be the most frequently accessed. * Revert "Refactor getFormat pattern and ordering for speed (smogon#5194)" This reverts commit 9ce0066. It doesn't account for bugs relating to Object.prototype * Refactor getEffect for speed safely (smogon#5201) Rollforward of 7a20245 which retains the `hasOwnProperty` checks. Also changes the method to call `toId` earlier and use the id as the key to the cache to ensure 'Stealth Rock' and 'stealthrock' return the same. NOTE: 'move: Stealth Rock' and 'Stealth Rock' will still continue to return different results. * Translation support (smogon#5167) * Fix field initialization ordering in Pokemon (smogon#5204) * Random Battle improvements * Random Battle: Properly increment tier counters * Gen I: Stop trying to change a fainted Pokemon's stat stages * Make sure Transform copies the correct type * Cleanup tsconfig includes further (smogon#5202) * Remove BH suspect * Fix misplaced string in PT translation * Fix an inaccurate simplified Chinese translation (smogon#5208) Bumba's OS not showing those characters somehow * Revert "Change (PU) to Untiered (smogon#5170)" This reverts commit d40fe51. * Formats: Update ladders * Change Untiered to (DUU) for Doubles (smogon#5178) * Don't add inactive rules as pseudoweathers (smogon#5206) * Fix crash with PMs * Pokebilities: Update Trace, Power of Alchemy, and Receiver (smogon#5203) * Pokebilities: Update Trace, Power of Alchemy, and Receiver Now Trace copies a random ability from a random target, regardless of whether it was originally an innate or not * Send correct target in message to client * Fill in Japanese aliases (smogon#5209) * Fix Gen 1 bug involving last damage and Substitute (smogon#2598) (smogon#5211) * Fix Counter/Mirror Coat to respect redirection (smogon#5212) * Refactor `sim/` to be native Typescript (smogon#5210) * Include data/mods/gen1/pokedex.js in tsconfig (smogon#5213) * Add GSC NU * Translations: fix missing english strings * Add one more volatile switch test * Run onModifyTemplate on rulesets (smogon#5207) * SSB: Arrested -> Pablo; Fix Fake Claim & Sub Interaction (smogon#5205) * Fix build/hotpatch process Hotpatching and running `./pokemon-showdown` now automatically run `./build`. There should now mostly not be any reason you'd want to manually run `./build`, except if you're invoking tests directly. In addition, a lot of redundant code has been removed. I'm not 100% sure this works on Windows, but I'm sure I'll get reports if anything breaks. * Explicitly import readdirSync in dex.ts This avoids ExperimentalWarnings caused by wildcard import of fs. * Update Psych Up and Transform interactions with crit rate effects * Datacenters: Add a range * Make !rules broadcastable This fixes a regression. * Fix fs.promises warning properly The modern `__esModule` fallback isn't compatible with Node, and also entirely unnecessary in practice. The approach of disabling all the fallback code allows us to once again `import * as fs`. * Fix crash in SSB import * Fix Shell Trap behavior with Encore (smogon#5218) * March RoA Spotlight * Make modlog asynchronous This should prevent issues in which the /modlog takes forever because it's waiting for previous /modlog command. * Log long modlog queries * Rephrase LICENSE GitHub couldn't identify the old license as the MIT license, so I'm just copy/pasting the one from the damagecalc which GitHub doesn't have a problem with. * RU: Add Zygarde-10% suspect notice * Datacenters: Add more ranges * Random Battle fixes * Update Other Metagames * Make sure suspect notices only show up in the relevant format * Update VS Code launch configurations - Remove "start server" because it's incompatible with multiprocess PS - Add "parse input log" * Fix Other Metagames of the Month (smogon#5224) * Tiers: Add March quick drops * Added RBY UU (smogon#5225) * Add BW Monotype * Megamons: Ban Mega Rayquaza And remove some unnecessary validator checks. * Change the RoA Spotlight DPP Ubers was wrongly announced. * Pull large avatarTable out into a Set constant. (smogon#5228) * Remove FIXME's in gen1/gen2 random-teams.js (smogon#5227) * Fix /dt showing 'undefined' as Zygarde ability (smogon#5153) * Refactor `lib/` to be native Typescript (smogon#5217) * Random Battle: Improve Kyurem * Fix Megamons and Chimera simplistically (smogon#5229) * Remove LC suspect notice (smogon#5231) * Update RU thread links (smogon#5230) * Refactor sim/dex-data.ts to minimize @ts-ignore (smogon#5214) * Fix crash in Anticipation (smogon#5232) A battle has crashed: TypeError: Cannot read property 'fainted' of null at Battle.onStart (/home/ps/showdown/data/abilities.js:150:16) * Fix Protean and Color Change (smogon#5233) * Fix dependency check in build script (This error wasn't caught because it still works without installing the dependency, but makes the build script far slower because `npx replace` would reinstall `replace` every time it's used.) * Fix validation of Gen 2 Marowak set This was a lot of work for what turned out to be a really simple fix. Gen 2 tradeback validation ended up being slightly too strict. * Use array spread instead of Array.from * LC: Unban Torchic (smogon#5234) https://www.smogon.com/forums/threads/torchic-has-been-freed.3647800/ * Disallow non-standard Pokemon in Gen1/2 randbats (smogon#5235) * Revert buggy Megamons "fix" (smogon#5236) * Fix HP bug in Chimera (smogon#5237) * Use FS when loading Scavengers' data (smogon#5238) This prevents issues with loading old state while hotpatching. * PU: Add Houndoom suspect notice * Chimera: Fix remaining HP bug (smogon#5240) * Fix `build` script on Windows. (smogon#5241) On Windows, `shell` doesn't glob for us and the literal `.sim-dist/*` is passed to `replace` and it doesn't get expanded into the list of paths. * Chimera: Fix ability bugs (smogon#5244) * Add suspect notice to Mix and Mega * Add Type: Null to Random Battle * Megamons: Fix bans
|
Seemingly related bug: https://lgtm.com/projects/g/Zarel/Pokemon-Showdown/rev/pr-c1f391939b8a933ed0608f0ab4a1b1b3da7983f3 (Is your fix live yet? Does anything need to be done for that other query to be manually re-enabled?) |
The fix is live, and I have silently enabled the query for your project again.
I am inclined to agree. But the initial values of those variables are indeed never read, and they could be ommitted. Our treatmentment of destructuring assignments is not as good as for ordinary assignments (that is: we would not have flagged the assignments if they were on the form: |
https://lgtm.com/projects/g/Zarel/Pokemon-Showdown/rev/pr-0f9a0e9926364b4e5ae570cd10428bd92603195e I don't know if this was overlooked or if it was a regression, but it looks like it's back. |
Hi there!
I thought you might be interested in adding these code quality badges to your project. They will indicate a very high code quality to potential users and contributors.
To get an idea of the analyses reflected by these grades, check the alerts discovered by LGTM.
N.B.: I am on the team behind LGTM.com, I'd appreciate your feedback on this initiative, whether you're interested or not, if you find time to drop me a line. Thanks.