Skip to content
This repository was archived by the owner on Mar 21, 2022. It is now read-only.

Fix for issue 26: Default to CMakeLists.txt in current directory#35

Open
shanebishop wants to merge 1 commit intorichq:masterfrom
shanebishop:issue-26-default-file-lint
Open

Fix for issue 26: Default to CMakeLists.txt in current directory#35
shanebishop wants to merge 1 commit intorichq:masterfrom
shanebishop:issue-26-default-file-lint

Conversation

@shanebishop
Copy link

No description provided.

@tkruse
Copy link
Contributor

tkruse commented Jun 1, 2019

The patch looks ok. I wonder though if this is good behavior in cases a project has split the build logic into multiple smaller .cmake files. The explicit command means when a file is not checked, it's the fault of the user. If we provide logic, it would become our fault.

Did you consider this case, and consider to also scanning for .cmake files?

@shanebishop
Copy link
Author

shanebishop commented Jun 1, 2019

@tkruse There are a couple things to consider:

  • Would we want to recursively search every subdirectory of the directory cmakelint is invoked in for all files matching CMakeLists.txt and *.cmake?
  • How ought we to differentiate between .cmake files that are generated by cmake itself, and .cmake files that are user-defined? It would be undesirable for users to be given warnings for .cmake files that were autogenerated by cmake.

@tkruse
Copy link
Contributor

tkruse commented Jun 1, 2019

I don't know myself. If the command is to try and guess the intention of a user who does not specify the file, we can only make reasonable assumptions and decide whether to be more bold or more conservative in including files.
I don't work with cmake enough to have any strong opinion on these questions, I am fine with merging the lines as they are now, just wanted to mention this possibility.

@shanebishop
Copy link
Author

I think this is probably a reasonable default for now.

@tkruse
Copy link
Contributor

tkruse commented Jun 8, 2019

copied to cmake-lint/cmake-lint#2

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants