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.

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

Actually fix Lint CI #6583

Closed
wants to merge 2 commits into from
Closed

Actually fix Lint CI #6583

wants to merge 2 commits into from

Conversation

somebody1234
Copy link
Contributor

Pull Request Description

My suspicions were correct... it seems like the failures mainly (if not only) happen on a clean repository without build artifacts.
I've fixed it so it will work anyway - it's not pretty but I think it's all required

Important Notes

None

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@somebody1234
Copy link
Contributor Author

@wdanilo you pointed out it was verbose before... well, it's so much more verbose now DX

the alternative solution would be to remove all this specialcasing, and then just check for the existence of e.g. build.json and run a build if it's not there. thoughts?

declare module '*/ensogl-pack/linked-dist' {
/* eslint-disable @typescript-eslint/consistent-type-imports, no-restricted-syntax, @typescript-eslint/no-shadow */
export type App = import('runner/index').App
export const App: typeof import('runner/index').App
export type Consumer = import('runner/index').Consumer
export const Consumer: typeof import('runner/index').Consumer
export type LogLevel = import('runner/index').LogLevel
export const LogLevel: typeof import('runner/index').LogLevel
export type Logger = import('runner/index').Logger
export const Logger: typeof import('runner/index').Logger
export type Option<T> = import('runner/index').Option<T>
export const Option: typeof import('runner/index').Option
export const log: typeof import('runner/index').log
export const logger: typeof import('runner/index').logger
export const urlParams: typeof import('runner/index').urlParams
export namespace config {
export type Option<T> = import('runner/index').config.Option<T>
export const Option: typeof import('runner/index').config.Option
export type Group<
Options extends OptionsRecord,
Groups extends GroupsRecord
> = typeof import('runner/index').config.Group<Options, Groups>
export const Group: typeof import('runner/index').config.Group
export const objectToGroup: typeof import('runner/index').config.objectToGroup
export const objectToOption: typeof import('runner/index').config.objectToOption
export const options: typeof import('runner/index').config.options
}
/* eslint-enable @typescript-eslint/consistent-type-imports, no-restricted-syntax, @typescript-eslint/no-shadow */
}

@somebody1234 somebody1234 added the CI: No changelog needed Do not require a changelog entry for this PR. label May 5, 2023
@somebody1234 somebody1234 force-pushed the wip/sb/really-fix-lint-ci branch from 71ccc1d to e23e65d Compare May 5, 2023 14:31
@somebody1234 somebody1234 changed the title *Actually* fix Lint CI Actually fix Lint CI May 5, 2023
@kazcw kazcw mentioned this pull request May 5, 2023
5 tasks
@wdanilo
Copy link
Member

wdanilo commented May 5, 2023

@wdanilo you pointed out it was verbose before... well, it's so much more verbose now DX

the alternative solution would be to remove all this specialcasing, and then just check for the existence of e.g. build.json and run a build if it's not there. thoughts?

declare module '*/ensogl-pack/linked-dist' {
/* eslint-disable @typescript-eslint/consistent-type-imports, no-restricted-syntax, @typescript-eslint/no-shadow */
export type App = import('runner/index').App
export const App: typeof import('runner/index').App
export type Consumer = import('runner/index').Consumer
export const Consumer: typeof import('runner/index').Consumer
export type LogLevel = import('runner/index').LogLevel
export const LogLevel: typeof import('runner/index').LogLevel
export type Logger = import('runner/index').Logger
export const Logger: typeof import('runner/index').Logger
export type Option<T> = import('runner/index').Option<T>
export const Option: typeof import('runner/index').Option
export const log: typeof import('runner/index').log
export const logger: typeof import('runner/index').logger
export const urlParams: typeof import('runner/index').urlParams
export namespace config {
export type Option<T> = import('runner/index').config.Option<T>
export const Option: typeof import('runner/index').config.Option
export type Group<
Options extends OptionsRecord,
Groups extends GroupsRecord
> = typeof import('runner/index').config.Group<Options, Groups>
export const Group: typeof import('runner/index').config.Group
export const objectToGroup: typeof import('runner/index').config.objectToGroup
export const objectToOption: typeof import('runner/index').config.objectToOption
export const options: typeof import('runner/index').config.options
}
/* eslint-enable @typescript-eslint/consistent-type-imports, no-restricted-syntax, @typescript-eslint/no-shadow */
}

Oh yes, let's do it as you propose please! Let's check for it, build it if it's not there and it'd be much better than this verbosity. Btw, why running build is needed like that? Our build script should build all dependencies automatically, so there should not be a situation when it's not there, right?

@somebody1234
Copy link
Contributor Author

@wdanilo i think it's because lint doesnt depend on build. i'm not very familiar with the build CLI so maybe it'd be better for @mwu-tow to do it?

@somebody1234 somebody1234 marked this pull request as draft May 5, 2023 18:07
@somebody1234
Copy link
Contributor Author

i think build_ide generates build.json and linked-dist (which are the exact things we need?)
however it seems like most of the defaults are coming from clap, so it's difficult to just build ide programmatically

Comment on lines +20 to +43
/* eslint-disable @typescript-eslint/consistent-type-imports, no-restricted-syntax, @typescript-eslint/no-shadow */
export type App = import('runner/index').App
export const App: typeof import('runner/index').App
export type Consumer = import('runner/index').Consumer
export const Consumer: typeof import('runner/index').Consumer
export type LogLevel = import('runner/index').LogLevel
export const LogLevel: typeof import('runner/index').LogLevel
export type Logger = import('runner/index').Logger
export const Logger: typeof import('runner/index').Logger
export type Option<T> = import('runner/index').Option<T>
export const Option: typeof import('runner/index').Option
export const log: typeof import('runner/index').log
export const logger: typeof import('runner/index').logger
export const urlParams: typeof import('runner/index').urlParams
export namespace config {
export type Option<T> = import('runner/index').config.Option<T>
export const Option: typeof import('runner/index').config.Option
export type Group<
Options extends OptionsRecord,
Groups extends GroupsRecord
> = typeof import('runner/index').config.Group<Options, Groups>
export const Group: typeof import('runner/index').config.Group
export const objectToGroup: typeof import('runner/index').config.objectToGroup
export const objectToOption: typeof import('runner/index').config.objectToOption
Copy link
Member

Choose a reason for hiding this comment

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

this is terribly verbose and hard to maintain. I understand that the second PR is an alternative to this one, without this verbosity. Am I correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah - i definitely agree the other one is cleaner... and more importantly, doesnt require manually adding or removing exports

@somebody1234
Copy link
Contributor Author

somebody1234 commented May 8, 2023

superseded by #6603

@somebody1234 somebody1234 deleted the wip/sb/really-fix-lint-ci branch May 8, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants