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

[Vue Rewrite] Clean up indentation + linting rules #1907

Merged
merged 4 commits into from
Oct 3, 2022

Conversation

devlinjunker
Copy link
Contributor

@devlinjunker devlinjunker commented Sep 16, 2022

PR to Cleanup Linting rules + Use Typescript in all Components so far

you can see the dev work done here

Up Next:

also clean up all lint warnings in js+vue files

Signed-off-by: Devlin Junker <[email protected]>
Signed-off-by: Devlin Junker <[email protected]>
@SMillerDev
Copy link
Contributor

I'd prefer if this was split into two reviews here too.

@devlinjunker
Copy link
Contributor Author

devlinjunker commented Sep 30, 2022

I moved the branch pointer back 2 commits so it doesn't include the Component to typescript changes.

This PR is now just for cleaning up indentation and linting and I have created a new PR for moving the component files to typescript: #1922

@devlinjunker devlinjunker changed the title Clean up indentation + linting rules / Move all Components to Typescript Clean up indentation + linting rules Sep 30, 2022
@devlinjunker devlinjunker changed the title Clean up indentation + linting rules [Vue Rewrite] Clean up indentation + linting rules Sep 30, 2022
Signed-off-by: Devlin Junker <[email protected]>
@SMillerDev
Copy link
Contributor

Why move to tabs by the way? Because all the other code in this repo uses spaces.

@devlinjunker
Copy link
Contributor Author

devlinjunker commented Sep 30, 2022

@SMillerDev I was just following the Vue style conventions rather than add a rule to overwrite them. If you think it makes sense to keep the consistency in the entire repo then I can re-add the:

rules: {
        'vue/html-indent': 'off',
        indent: ['error', 4],
    },

to the .eslintrc file and run the lint again

@Grotax Grotax added the Skip-Changelog No changelog update is required, minor change label Sep 30, 2022
Copy link
Member

@Grotax Grotax left a comment

Choose a reason for hiding this comment

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

Looks ok to me, I don't mind tabs or spaces as long as it is consistent.
And if tabs is the default for this linter then we can stick to that.

Modern IDEs detect that automatically anyway.

Copy link
Contributor

@anoymouserver anoymouserver left a comment

Choose a reason for hiding this comment

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

I agree with Grotax that tabs are ok too, as long as it's consistent per file type.
However, please also modify the .editorconfig, otherwise the next IDE/editor would simply change it back to spaces.

Remove js from here

news/.editorconfig

Lines 8 to 11 in 3ff51b5

[*.{js,php,html}]
indent_style = space
indent_size = 4
charset = utf-8

... and add a block like this

[*.{js,ts,vue}]
indent_style = tab
indent_size = 4
charset = utf-8

@devlinjunker
Copy link
Contributor Author

Updated the .editorconfig and those remaining spaces in the vue html @anoymouserver

@devlinjunker
Copy link
Contributor Author

devlinjunker commented Oct 3, 2022

after this is merged, we can move on to #1922

and I am currently working on adding unit tests for the existing components. See devlinjunker#56 for the WIP changes so far

@Grotax Grotax merged commit a71fc67 into nextcloud:vue-rewrite Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip-Changelog No changelog update is required, minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants