Skip to content

Normalize code formatting#579

Open
hyfloac wants to merge 1 commit intoNiek:mainfrom
hyfloac:normalize-formatting
Open

Normalize code formatting#579
hyfloac wants to merge 1 commit intoNiek:mainfrom
hyfloac:normalize-formatting

Conversation

@hyfloac
Copy link

@hyfloac hyfloac commented May 19, 2025

I want to start by saying, I don't like doing this, but I think it's the better of several bad options.

The codebase has some inconsistencies in its formatting. It seems pretty apparent what the target format was, but it doesn't always follow it. If left as is, any future changes I make would either have to straddle the variances in formatting (such as inconsistent indent depths), or have to fix the formatting leaving unrelated changes in the commit.

In the interest of making this as maintainable as possible for everybody, I decided to simply fix all of the inconsistencies I could find in one large commit, and allow all future changes to be isolated to their necessary changes.

I've tried my best to keep this in line with what the target format appears to be.

Here is a summary of the changes:

  • The code had non-uniform formatting all throughout. Some blocks had too few indents, some had not enough. As best as I could tell the target style was 2 indents for every block.
  • There were also some places where types did not have a space after the colon, and others where there was a space, it seemed most likely that the intent was to have a space.
  • Some type unions had spaces around the pipes, some did not, overall it seemed that the intent was to have spaces.
  • A couple pieces of HTML had been wrapped oddly, those were corrected to follow a consistent 2 indents.
  • A couple of long lines were also wrapped.
  • In a couple of places where functions had many parameters that were wrapped, a newline was added after the last parameter, and the closing parenthesis was left-dented to be inline with the column of the beginning of the function declaration.
  • All newlines before the closing script tag were removed.
  • Newlines were added to the end of every file, some editors behave oddly when there isn't that newline there.

Unfortunately, this basically nukes the blame history, making it difficult to see the original source of the changes. Fortunately, if someone does need to trace the source of a change, they can view the code at the commit before this, and inspect the blame.

Again, I don't like doing this, but I do have more changes planned, and I try to keep PRs isolated to a single change.

@hyfloac hyfloac force-pushed the normalize-formatting branch from 52d62c0 to a5099fb Compare May 19, 2025 03:35
@Niek
Copy link
Owner

Niek commented May 19, 2025

Thanks for the PR. I have a linting step in the package JSON to keep the formatting consistent. If you feel the formatting is wrong, it's better to change the ESLint rules than to change the formatting manually? Now there is no way to apply these rules to any new changes.

@hyfloac hyfloac force-pushed the normalize-formatting branch from a5099fb to 22b29d1 Compare May 19, 2025 21:34
The code had non-uniform formatting all throughout. Some blocks had too few indents, some had not enough. As best as I could tell the target style was 2 indents for every block.
There were also some places where types did not have a space after the colon, and others where there was a space, it seemed most likely that the intent was to have a space.
Some type unions had spaces around the pipes, some did not, overall it seemed that the intent was to have spaces.
A couple pieces of HTML had been wrapped oddly, those were corrected to follow a consistent 2 indents. A couple of long lines were also wrapped.
In a couple of places where functions had many parameters that were wrapped, a newline was added after the last parameter, and the closing parenthesis was left-dented to be inline with the column of the beginning of the function declaration.
All newlines before the closing script tag were removed. Newlines were added to the end of every file, some editors behave oddly when there isn't that newline there.
@hyfloac hyfloac force-pushed the normalize-formatting branch from 22b29d1 to 4fe222c Compare May 19, 2025 21:38
@hyfloac
Copy link
Author

hyfloac commented May 19, 2025

Ahh, I'm not sure if that command is currently working.

Whenever I run that I get the message:

=============

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.3.1 <5.2.0

YOUR TYPESCRIPT VERSION: 5.4.5

Please only submit bug reports when using the officially supported version.

=============

The TypeScript version for the repo is set to <5.5, which seems to result in the locked version being at 5.4.5.

After running the command no changes appear to be made. I'm unsure if this is because of the rules or the version mismatch. Some testing did indicate that it would still detect and fix some problems though.


I figure I should provide a clear example on some of the non-uniformity I've noticed. As an example:

src/lib/providers/openai/util.svelte
<script context="module" lang="ts">
    import { apiKeyStorage, getApiBase } from '../../Storage.svelte'
    import { get } from 'svelte/store'
    import type { ModelDetail } from '../../Types.svelte'
    import { getEndpointModels } from '../../ApiUtil.svelte'

type ResponseModels = {
  object?: string;
  data: {
    id: string;
  }[];
}

let availableModels: Record<string, boolean> | undefined

let _resetSupportedModelsTimer

export const set = (opt: Record<string, any>) => {
  availableModels = undefined
  apiKeyStorage.set(opt.apiKey || '')
}

export const getSupportedModels = async (): Promise<Record<string, boolean>> => {
  if (availableModels) return availableModels
  const openAiKey = get(apiKeyStorage)
  if (!openAiKey) return {}
  try {
        const result = (await (
          await fetch(getApiBase() + getEndpointModels(), {
            method: 'GET',
            headers: {
              Authorization: `Bearer ${openAiKey}`,
              'Content-Type': 'application/json'
            }
          })
        ).json()) as ResponseModels
        availableModels = result.data.reduce((a, v) => {
          a[v.id] = v
          return a
        }, {})
        return availableModels
  } catch (e) {
        console.error(e)
        availableModels = {}
        clearTimeout(_resetSupportedModelsTimer)
        _resetSupportedModelsTimer = setTimeout(() => { availableModels = undefined }, 1000)
        return availableModels
  }
}

export const checkModel = async (modelDetail: ModelDetail) => {
  const supportedModels = await getSupportedModels()
  if (modelDetail.type === 'chat' || modelDetail.type === 'instruct') {
        modelDetail.enabled = !!supportedModels[modelDetail.modelQuery || '']
  } else {
        // image request.  If we have any models, allow image endpoint
        modelDetail.enabled = !!Object.keys(supportedModels).length
  }
}

</script>

Lines 2-5 indent 4 spaces, but most other files seem to target 2 spaces.

The code from lines 7-21 starts indented from 0 spaces, but most other files seem to target 2 spaces.

The function at lines 23-49 (getSupportedModels) starts indented from 0 spaces, but then the contents of the try-catch block suddenly indents to column 8, most other files would indicate that if it is starting at column 0, then it should be at column 4, or if the function started at column 2 (as would seem to be the overall intent), then it should be at column 6.

A similar effect is seen in the if-else block of the function at lines 51-59 (checkModel).


Presumably, given eslint will fix some problems, this is caused by eslint being underconfigured.

At the end of the day, it's your code, so you get to dictate the style, and thus I think it would be best if you chose the rules in the eslint config. My goal here is not to impose a specific style upon your code (with the exception of the trailing new lines, which leaned more towards not having them, but it was also non-uniform), I'm simply trying to normalize the code to fit what would appear to be your preferred style.

As I said, I like to try to keep PRs isolated, and that is why I cut this out from the rest of my changes. If you would like to keep it as is, I can certainly work around that, but I cannot guarantee that my code will follow your style, since it would be unclear what the rules are, and eslint doesn't seem to impose anything specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants