Skip to content

Introduce ESLint #82

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

Closed
wants to merge 9 commits into from
Closed

Introduce ESLint #82

wants to merge 9 commits into from

Conversation

boesing
Copy link
Member

@boesing boesing commented Mar 16, 2022

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
QA yes

Description

Adds ESLint to this project to provide at least some CI pipeline for the moment.

boesing added 5 commits March 16, 2022 19:48
Signed-off-by: Maximilian Bösing <[email protected]>
… the latest version available

Signed-off-by: Maximilian Bösing <[email protected]>
@boesing boesing mentioned this pull request Mar 16, 2022

dependencies = 'locked';

ignorePhpPlatformRequirement = false;
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these changes potential BC breaks?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say no as we do not provide an API here, right?
Only changing application code. Input configs and output matrix havent changed.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this serialized and given to the next action?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, there is a dedicated method toJSON for that.

return [new Job(
command + ' on PHP ' + config.minimum_version,
return [ new Job(
`${command } on PHP ${ config.minimumVersion}`,
JSON.stringify(new Command(
Copy link
Member

Choose a reason for hiding this comment

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

This JSON.stringify() will produce a data structure that is potentially quite different from what we had before 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

why do you think so? cant really follow.
JSON.stringify is already part of the latest version?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I mean that the structure passed to it will be a bit different :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see what you mean and yes indeed, it changes a little bit.
Good catch. We are missing to call toJSON here.

There is some "normalization" stuff going on in another function which converts the string back to an object and verifies properties.
There it will not properly match anymore. 👍🏼

@Ocramius Ocramius added this to the 1.12.0 milestone Mar 16, 2022
@boesing boesing requested a review from Ocramius March 16, 2022 22:03
@Ocramius Ocramius requested a review from weierophinney March 24, 2022 01:23
@boesing
Copy link
Member Author

boesing commented Mar 24, 2022

Currently working on switching to typescript while also restructuring the whole application.
Making some progress there but still not finished - I am not too familiar with typescript but I tried to get some feedback from known typescript devs.
Main idea would be to get rid of all the annotation magic and getting better overview of what is happening.

Will see if I can finish #62 with some integration tests of "projects" so we have a bunch of tests which can also be executed by the new typescript version of this application.

If that is something we are fine with, I'd close this PR to instead use tslint in the typescript version.
Cool?

@boesing boesing marked this pull request as draft March 24, 2022 09:31
@boesing boesing closed this Mar 24, 2022
@boesing boesing mentioned this pull request Mar 25, 2022
7 tasks
@Ocramius Ocramius deleted the qa/eslint branch August 18, 2022 11:52
@renovate renovate bot mentioned this pull request Oct 29, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants