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

Apply minor CodeFactor code quality fixes #5762

Closed
wants to merge 2 commits into from

Conversation

dan-giddins
Copy link

Minor code quality changes recommended and auto-generated by https://www.codefactor.io/

@LmmsBot
Copy link

LmmsBot commented Nov 1, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://11264-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.37%2Bg331644d-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11264?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://11261-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.37%2Bg331644daa-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11261?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://11262-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.37%2Bg331644daa-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11262?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/p2lntmb8q7s03ygc/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36626255"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/p4b7ge38wb61ai63/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36626255"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/msgj7dpaga5m87ar/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37322915"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/m6yspuekwimg4an9/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37322915"}]}, "commit_sha": "2420d5f16323157617e207012f7a062e172b2a80"}

@dan-giddins
Copy link
Author

dan-giddins commented Nov 1, 2020

Could we add https://www.codefactor.io/ to LMMS as a build check? It really is rather good and is free for public projects.

@dan-giddins dan-giddins changed the title Apply CodeFactor fixes Apply minor CodeFactor code quality fixes Nov 1, 2020
@dan-giddins
Copy link
Author

Tested locally, and everything seems fine. It might be worth someone with a bit more experience checking through things, but from my limited knowledge of XML and CSS, the changes codefactor has recommended seem semantically equivalent.

@PhysSong
Copy link
Member

PhysSong commented Nov 2, 2020

Changes in .css files looks generally good. However, I think .ts files shouldn't be touched because that might break translations. If a source string contains trailing whitespaces, that should be fixed in the corresponding C++ code. @liushuyu Is this right?

@IanCaio
Copy link
Contributor

IanCaio commented Nov 3, 2020

LGTM too (been a while I don't use CSS, so I didn't remember the three values option, thought it was either 1, 2 or 4 😄). As for what PhysSong said, if the source checks are sensible to trailing whitespaces the changes on those files could be an issue. Have to check that first, and if it breaks translations the only way to fix those is to manually fix all the source strings in the source code (could use some sed script to make it easier, still would be a bit workful).

@PhysSong
Copy link
Member

PhysSong commented Nov 4, 2020

It seems like #4128 has removed relevant translation strings which existed in plugins/monstro/Monstro.cpp.
@liushuyu What should we do with those strings in .ts files?

@liushuyu
Copy link
Member

Sorry for the late reply!

Changes in .css files looks generally good. However, I think .ts files shouldn't be touched because that might break translations. If a source string contains trailing whitespaces, that should be fixed in the corresponding C++ code. @liushuyu Is this right?

You are absolutely right! The translation strings should be verbatim versions of the strings in the source code files. Otherwise, the translation will be missing. To resolve this issue, corresponding C/C++ source files should be edited and then you can regenerate a set of new translation files.

It seems like #4128 has removed relevant translation strings which existed in plugins/monstro/Monstro.cpp.
@liushuyu What should we do with those strings in .ts files?

If they've been removed in the source code, they should be removed in the .ts files as well. But don't worry, they will not disappear completely, the translation memory in Transifex will still remember them.

@dan-giddins
Copy link
Author

dan-giddins commented Dec 2, 2020

I have reverted the changes regarding the translations

@tresf
Copy link
Member

tresf commented Dec 8, 2020

@dan-giddins can you please assist us toggling this tool on for just the items we won't be covering with #4690? For example, this PR makes CSS more consistent and we'd certainly like to have a CI check that handles this and the PR is small enough to be able to turn that on immediately.

Sorry for the delay. I have some reservations about using shorthand syntax over readable syntax, but in the end, consistency wins. I've un-archived the codefactor project. Happy to add you as a maintainer if it's needed to get the settings right. For now I've diabled .ts files, but I don't think that's enough.

@PhysSong
Copy link
Member

@tresf So What should we do with this PR?

@tresf
Copy link
Member

tresf commented Jan 15, 2021

@tresf So What should we do with this PR?

Closing until/unless the OP comes back to help with our implementation.

@tresf tresf closed this Jan 15, 2021
@dan-giddins
Copy link
Author

@tresf Sorry for the delay.

Yes I think I will need to be a maintainer to add it to the project. You could of course do it yourself if you want to limit the number of maintainers on the project. Either way, what we would need to do would be to use GitHub checks.

I've installed it for my personal profile:
image
You could try adding me as a maintainer, and it might just automatically work, but it might need to be the owner who installs it...

@tresf tresf reopened this Jan 18, 2021
@tresf
Copy link
Member

tresf commented Jan 18, 2021

Why does it need to administer our project? That seems rather extreme to be granting to a 3rd party service.

@tresf
Copy link
Member

tresf commented Jan 18, 2021

Invite sent. Hopefully write access is enough (the codefactor portal suggests it is).

@dan-giddins
Copy link
Author

@tresf is this what you are talking about?
image
If we want to use this we need to allow codefactor to have the following access:
image
I didn't want to press 'Approve & Request' incase this just adds it without anyone else approving. Can I do this? Does anyone know what pressing 'Approve & Request' does?
I think that Codefactor is now installed as a 'Check', but if we want annotations, we need to install the github app

@tresf
Copy link
Member

tresf commented Jan 18, 2021

You can do it, I believe it's read-only and your permissions don't have access to the dangerous stuff. Oddly, I've already done all of this using the service account (there was a bit of ambiguity as to WHO's access I was granting), so I think it's actually asking about this on your own profile. Either way, go ahead and proceed. What I don't like about read-only access to the administrative functions is it may be granting access to some private keys we use for webhooks, something I wouldn't want codefactor to have access to (and I'd expect them to share the same sentiments).

@dan-giddins dan-giddins closed this Nov 5, 2023
@dan-giddins dan-giddins deleted the master-cf-autofix branch November 5, 2023 14:24
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.

7 participants