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

New formats support #632

Merged
merged 4 commits into from
Feb 8, 2023
Merged

New formats support #632

merged 4 commits into from
Feb 8, 2023

Conversation

sgaist
Copy link
Contributor

@sgaist sgaist commented Dec 4, 2022

This merge request implements a couple of requested format support.

Each format is in it's own commit to keep things clearly separated.

Fixes #575
Fixes #614
Fixes #616

@nicorikken
Copy link
Member

@sgaist Thanks a lot! Reading up on .clang-format I see this is supposed to be the entire filename, not the extension. Also the _clang-format filename is a valid alternative. I added it in #575 perhaps you can add that to that commit? Otherwise I think this looks good.

@sgaist
Copy link
Contributor Author

sgaist commented Dec 4, 2022

Sure thing !

Should we consider adding support for other tools configuration files like .flake8, .bandit and the likes ?

@nicorikken
Copy link
Member

If you have suggestions, please do. Please provide some examples or references in the commit or discussion so we can refer to it at a later stage.

@sgaist
Copy link
Contributor Author

sgaist commented Dec 4, 2022

Shouldn't I rather open an issue about that topic so we can discuss there the list of tools ?

@nicorikken
Copy link
Member

@sgaist yeah that would probably be best.

@sgaist
Copy link
Contributor Author

sgaist commented Dec 6, 2022

In this case, should I remove the .clang-format commit from this PR and put it back when the tools file support discussion is done ?

@carmenbianca
Copy link
Member

You can put it in FILENAME_COMMENT_STYLE_MAP.

_clang-format I am not sure about.

@sgaist
Copy link
Contributor Author

sgaist commented Dec 6, 2022

Thanks @carmenbianca I somehow missed that one. Will be perfect for #633.

Copy link
Member

@carmenbianca carmenbianca left a comment

Choose a reason for hiding this comment

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

LGTM

@sgaist
Copy link
Contributor Author

sgaist commented Feb 7, 2023

One thing I just realized, I have listed the specific issues per commit which can trigger quite a lot of noise on their respective page.

I think I should rewrite the message of these commits and only mention the issues in the pull request.

Agreed ?

@sgaist
Copy link
Contributor Author

sgaist commented Feb 7, 2023

I fixed the pylint issue and did the message change at the same time.

@carmenbianca
Copy link
Member

Added a change log. Will merge when tests succeed. Thank you @sgaist !

sgaist and others added 4 commits February 8, 2023 23:17
.mjs is already present
This should also cover the proguard-rules.pro file mentionned in
ticket 614
Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
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.

Support for .cjs and .mjs files Comment style for proguard rules reuse should detect .clang-format files
4 participants