OpenVAA / voting-advice-application

An open-source platform for creating Voting Advice Applications (VAAs)
https://demo.openvaa.org/
GNU General Public License v3.0
8 stars 0 forks source link

chore: reformat code globally #510

Open kaljarv opened 1 month ago

kaljarv commented 1 month ago

Harmonize code formatting globally:

  1. Import statements and curly brace spacing
  2. Import ordering
  3. Convert string unions to Enums
  4. Comments
    1. Remove non-paragraph line breaks from comments
    2. Remove @default statements from @component docstrings because they break the IDE layout
  5. Naming conventions
  6. Consider changing all explicit undefineds to nulls if this is the recommended approach. At least, null is compatible with JSON, which is a plus.
    1. This may require globally changing optional Svelte component properties to accept null and undefined, which ought to be checked as well.
    2. MISSING_ANSWER is now defined as undefined (pun not intended) in vaa-matching, but null might be a better option because of JSON compatibility.
  7. Check all cases where setTimeout is used to wait for DOM updates and see if those can be converted to use tick.
  8. Convert components events to use Svelte-5-style callback props instead of eventDispatcher.
kaljarv commented 1 month ago

A way to start is to look at the style guides of existing OS projects (with TS)

norppaa commented 4 weeks ago

Undefined vs null

It seems that the convention in TypeScript is that undefined values have not been defined yet, whereas null values indicate intentional absence of a value. Therefore, I would not just change all undefineds to null, because they have different meanings.

Some sources say that the use of undefined is recommended and null is discouraged (all these cite the coding guidelines for contributors of typescript but this does not necessarily mean that projects made with typescript should only use undefined). Many seem to prefer using only undefined.

Some also argue, that you should not assign undefined to any value, because it is easier to debug programs if you know that if you get undefined it was because the variable never got set. Null indicates a variable that is empty or contains nothing (but has type etc)

https://www.reddit.com/r/typescript/comments/11dpu05/undefined_vs_null/

I could not really find any repos or style guides that take a stand on what should be used. Google and ts.dev styling guide both say that many js apis use undefined and many dom and google apis use null, so the appropriate value depends on the context.

See also TS Style Guide.

Enums vs string unions Why is it not good to use enums comparison

Why use string literals:

Why use Enums:

Different ways to use enums so that they safe typesafety

norppaa commented 3 weeks ago

Quite extensive type-script style guide

kaljarv commented 2 weeks ago

The style guide looks like something we could adopt, although these rules might need discussion:

We might need some automatic type generation for services and the like, e.g. Strapi types and translations. It'd be worthwhile to investigate how this can be best achieved.

anolpe commented 2 weeks ago

Do we have lot of cases where we have to use interfaces?

I think I often use string[] but I don't have a strong preference.

What kind of linter issues?

Agree, and in those cases it's pretty clear that those are boolean.

kaljarv commented 2 weeks ago

Not necessarily that many, and most all can be converted into types. Possible contexts where interfaces are needed are:

Me too :D, but I'm starting to think that Array<string> might be preferable because it's easier to distinguish from string. An added benefit is a less messy initialisation for empty typed arrays:

const foo = new Array<Bar>();
// vs.
const foo: Bar[] = [];

I had such a discriminated construction for the ButtonProps type in $lib/compontents/icon, which only allowed for some values of iconPos for some variants. It was easy enough to describe in the interface, but then on some pages where the component is used with variables as the values for these properties, the linter would rightly complain it couldn't be sure the combination was correct. In theory, this could be worked around but that seemed a bit complicated, but it would ensure correct usage on the other hand 🤔 . E.g.

let foo: ButtonProps['variant'];
let bar: ButtonProps['iconPos'];

// Set values here

// This will still be bad, because we don't know if `foo` and `bar` are compatible
<Button variant={foo} iconPos={bar}/>
// Could be possibly worked around this way or so
let props: Partial<ButtonProps> = foobar
  ? { variant: 'iconAnywhere', iconPos: 'left' } 
  : { variant: 'iconOnlyRight', iconPos: 'right' };

<Button {...props}/>
kaljarv commented 1 week ago