tarantool / checkpatch

Checkpatch for Tarantool
GNU General Public License v2.0
2 stars 2 forks source link

Impossible to import third_party/*.lua #41

Closed tsafin closed 1 year ago

tsafin commented 1 year ago

If we try to import 3rd-party lua module first as-is, without any local modifications, and then only rollover all our local changes, then this PR would fail checkpatch linter, as original files to be imported, for obvious reasons, were not compliant to our linter rules set.

https://github.com/tarantool/tarantool/actions/runs/3083642178/jobs/4984821296 (https://github.com/tarantool/tarantool/pull/7593)

ERROR:TRAILING_WHITESPACE: trailing whitespace
#2379: FILE: third_party/lua/luadebug_tutorial.lua:235:
+^I$

ERROR:TABSTOP: please, use spaces instead of tabs
#2379: FILE: third_party/lua/luadebug_tutorial.lua:235:
+^I$

ERROR:TABSTOP: please, use spaces instead of tabs
#2380: FILE: third_party/lua/luadebug_tutorial.lua:236:
+^IType 'c' to continue.$

We should skip formatting checks for third_party/

locker commented 1 year ago

@tsafin I see that you reformat the imported code to conform to the checkpatch rules in a separate commit. Can you do it in the original commit? I don't see much value in splitting the changes.

As for suppressing the checks for third_party/ files - I'm not sure we should do that. The checks are generic and should be valid for any project (no trailing spaces, no mixing spaces and tabs, etc). We don't do any codestyle-specific checks there.

ligurio commented 1 year ago

@tsafin I see that you reformat the imported code to conform to the checkpatch rules in a separate commit. Can you do it in the original commit? I don't see much value in splitting the changes.

On code review, we decided to split commit with original code and commits with code style changes. This splitting simplifies code review made by Timur and ignore problems that are contained in the original code.

The checks are generic and should be valid for any project (no trailing spaces, no mixing spaces and tabs, etc). We don't do any codestyle-specific checks there.

I agree that we should keep our code in a common code style. But third_party dir contains a lot of code derived from other projects with their own rules and code styles. If we will follow your rule then we need to update source code of every third party component on the first commit, even we don't plan to introduce functional changes. It looks like we want to introduce code style for code style and looks useless.

In addition, reformatting third-party source code will make rebasing to upstream more complex.

locker commented 1 year ago

The checks are generic and should be valid for any project (no trailing spaces, no mixing spaces and tabs, etc). We don't do any codestyle-specific checks there.

I agree that we should keep our code in a common code style. But third_party dir contains a lot of code derived from other projects with their own rules and code styles. If we will follow your rule then we need to update source code of every third party component on the first commit, even we don't plan to introduce functional changes. It looks like we want to introduce code style for code style and looks useless.

@ligurio These checks aren't about coding style - they're sanity text formatting checks (warn about DOS line endings, trailing spaces, usage of tabs instead of spaces in text files). I'd rather keep them, because they make sense for any text file committed to the Tarantool repository. If you're certain that splitting code import and reformatting is better than doing both in the same commit (although I personally disagree in this particular case), we can merge the PR despite the checkpatch warnings (it's acceptable, although frowned upon, because the checkpatch workflow isn't run on the master branch).