smogon / Porygon-Z

The Bot for the Official Smogon Discord (WIP)
https://discord.gg/smogon
MIT License
13 stars 6 forks source link

Use ESLint and GitHub Actions #24

Closed AnnikaCodes closed 3 years ago

AnnikaCodes commented 3 years ago

I mostly borrowed this from PS (removing some PS-specific stuff); I assume that all the @smogon projects should probably have similar ESLint configurations.

Fixes #2

AnnikaCodes commented 3 years ago

@HoeenCoder bump

AnnikaCodes commented 3 years ago

@HoeenCoder bump

HoeenCoder commented 3 years ago

Hi sorry.

I don't know what you want to do about all of your hanging news, which cause ESLint errors. Probably you should construct it and then call .initialize() to actually display the page?

Probably would be the best way honestly. Pages feel a bit messy to me atm, might have them worked on later to improve them.

ICommandModule baffles me. It's an object whose values are either Constructable or string[], but you seem to treat it as though it sometimes also contains another object containing information about command aliases? This causes another ESLint error.

The concept is its the exports from a command file. Each item exported will either be a BaseCommand subclass or an object with keys as strings and values as string arrays (alias objects, generally 1 per file). ICommandModule refers to any one item from a file. Not sure the best way to handle this.

AnnikaCodes commented 3 years ago

The concept is its the exports from a command file. Each item exported will either be a BaseCommand subclass or an object with keys as strings and values as string arrays (alias objects, generally 1 per file). ICommandModule refers to any one item from a file. Not sure the best way to handle this.

I think this is what you're looking for:

type CommandAliases = {[key: string]: string[]};
type ICommandModule = {[key: string]: Constructable<BaseCommand> | CommandAliases};
AnnikaCodes commented 3 years ago

I changed Pages around a little to remove hanging news. (This also properly handles promises, which isn't something this codebase does very often—if one doesn't want to block on a Promise, usually it gets marked with void to denote that.)

HoeenCoder commented 3 years ago

(This also properly handles promises, which isn't something this codebase does very often—if one doesn't want to block on a Promise, usually it gets marked with void to denote that.)

Was unaware of the use of void in JS/TS bar return type honestly or that we had it available. My only experience is with it in Java/C#. That should be changed as appropriate then, possibly the linter could enforce that too?

AnnikaCodes commented 3 years ago

I'm pretty sure the linter does enforce it—I made sure @typescript-eslint/no-floating-promises was enabled.

The MDN docs for void are honestly kind of unrelated to its use with async/await. With Promises, it's usually used to make it clear that a Promise is being returned by the function call, but we don't want to await it (typically to avoid blocking the function we're in), so we explicitly force it undefined (which basically ignores the Promise and lets it execute on its own).

HoeenCoder commented 3 years ago

Other than that all looks good