Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .gersemirc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# yamllint disable-line rule:line-length
# yaml-language-server: $schema=https://raw.githubusercontent.com/BlankSpruce/gersemi/master/gersemi/configuration.schema.json

line_length: 100
list_expansion: "favour-expansion"
34 changes: 34 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# CMake 3.22.1 is the default on Ubuntu 22.04
cmake_minimum_required(VERSION 3.22.1)

project(
spider
LANGUAGES
C
CXX
VERSION 0.1.0
)

# Enable exporting compile commands
set(CMAKE_EXPORT_COMPILE_COMMANDS
ON
CACHE BOOL
"Enable/Disable output of compile commands during generation."
FORCE
)

# Set the default build type to Release if not specified
if(NOT CMAKE_BUILD_TYPE)
set(SPIDER_DEFAULT_BUILD_TYPE "Release")
message(STATUS "No build type specified. Setting to '${SPIDER_DEFAULT_BUILD_TYPE}'.")
set(CMAKE_BUILD_TYPE
"${SPIDER_DEFAULT_BUILD_TYPE}"
CACHE STRING
"Choose the type of build."
FORCE
)
endif()
Comment on lines +20 to +30

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding build type options validation.

The build type configuration could benefit from validating allowed values and documenting available options.

 if(NOT CMAKE_BUILD_TYPE)
     set(SPIDER_DEFAULT_BUILD_TYPE "Release")
     message(STATUS "No build type specified. Setting to '${SPIDER_DEFAULT_BUILD_TYPE}'.")
     set(CMAKE_BUILD_TYPE
         "${SPIDER_DEFAULT_BUILD_TYPE}"
         CACHE STRING
-        "Choose the type of build."
+        "Choose the type of build (Debug/Release/RelWithDebInfo/MinSizeRel)."
         FORCE
     )
+    # Provide drop-down in CMake GUIs
+    set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS
+        "Debug" "Release" "RelWithDebInfo" "MinSizeRel")
 endif()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Set the default build type to Release if not specified
if(NOT CMAKE_BUILD_TYPE)
set(SPIDER_DEFAULT_BUILD_TYPE "Release")
message(STATUS "No build type specified. Setting to '${SPIDER_DEFAULT_BUILD_TYPE}'.")
set(CMAKE_BUILD_TYPE
"${SPIDER_DEFAULT_BUILD_TYPE}"
CACHE STRING
"Choose the type of build."
FORCE
)
endif()
# Set the default build type to Release if not specified
if(NOT CMAKE_BUILD_TYPE)
set(SPIDER_DEFAULT_BUILD_TYPE "Release")
message(STATUS "No build type specified. Setting to '${SPIDER_DEFAULT_BUILD_TYPE}'.")
set(CMAKE_BUILD_TYPE
"${SPIDER_DEFAULT_BUILD_TYPE}"
CACHE STRING
"Choose the type of build (Debug/Release/RelWithDebInfo/MinSizeRel)."
FORCE
)
# Provide drop-down in CMake GUIs
set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS
"Debug" "Release" "RelWithDebInfo" "MinSizeRel")
endif()


add_executable(spider)
target_compile_features(spider PRIVATE cxx_std_20)
target_sources(spider PRIVATE src/spider/spider.cpp)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider organizing target properties more systematically.

The target configuration could be enhanced with:

  • Organized property settings
  • Include directories specification
  • Compile definitions/options
 add_executable(spider)
+
+# Specify C++ standard through a target property
 target_compile_features(spider PRIVATE cxx_std_20)
+
+# Group source files for better organization
+set(SPIDER_SOURCES
+    src/spider/spider.cpp
+)
+target_sources(spider PRIVATE ${SPIDER_SOURCES})
+
+# Add include directories if needed
+target_include_directories(spider
+    PRIVATE
+        ${CMAKE_CURRENT_SOURCE_DIR}/src
+)

Committable suggestion was skipped due to low confidence.

10 changes: 6 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ The commands above run all linting checks, but for performance you may want to r
if you only changed C++ files, you don't need to run the YAML linting checks) using one of the tasks
in the table below.

| Task | Description |
|-------------------------|----------------------------------------------------------|
| `lint:yml-check` | Runs the YAML linters. |
| `lint:yml-fix` | Runs the YAML linters and fixes some violations. |
| Task | Description |
|--------------------|--------------------------------------------------|
| `lint:cmake-check` | Runs the CMake linters. |
| `lint:cmake-fix` | Runs the CMake linters and fixes any violations. |
| `lint:yml-check` | Runs the YAML linters. |
| `lint:yml-fix` | Runs the YAML linters and fixes some violations. |

[Task]: https://taskfile.dev
1 change: 1 addition & 0 deletions lint-requirements.txt
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
gersemi>=0.16.2
yamllint>=1.35.1
25 changes: 25 additions & 0 deletions lint-tasks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,28 @@ vars:
tasks:
check:
cmds:
- task: "cmake-check"
- task: "yml-check"

fix:
cmds:
- task: "cmake-fix"
- task: "yml-fix"

cmake-check:
deps: ["venv"]
cmds:
- task: "cmake"
vars:
FLAGS: "--check"

cmake-fix:
deps: ["venv"]
cmds:
- task: "cmake"
vars:
FLAGS: "--in-place"

yml:
aliases:
- "yml-check"
Expand All @@ -22,6 +38,14 @@ tasks:
. "{{.G_LINT_VENV_DIR}}/bin/activate"
yamllint --strict .

cmake:
internal: true
requires:
vars: ["FLAGS"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find . -name CMakeLists.txt -print0 | xargs -0 gersemi {{.FLAGS}}

Comment on lines +41 to +52

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing the cmake task's robustness.

While the implementation is functional, consider these improvements:

  1. Add directory exclusions (e.g., build directories, third-party code)
  2. Add error handling for the case when no CMakeLists.txt files are found

Here's a suggested improvement:

-      find . -name CMakeLists.txt -print0 | xargs -0 gersemi {{.FLAGS}}
+      find . -name CMakeLists.txt \
+           -not -path "*/build/*" \
+           -not -path "*/third_party/*" \
+           -print0 | \
+      xargs -0 -r gersemi {{.FLAGS}}

The -r flag for xargs ensures it won't run if no files are found.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cmake:
internal: true
requires:
vars: ["FLAGS"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find . -name CMakeLists.txt -print0 | xargs -0 gersemi {{.FLAGS}}
cmake:
internal: true
requires:
vars: ["FLAGS"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find . -name CMakeLists.txt \
-not -path "*/build/*" \
-not -path "*/third_party/*" \
-print0 | \
xargs -0 -r gersemi {{.FLAGS}}

venv:
internal: true
vars:
Expand All @@ -32,6 +56,7 @@ tasks:
- "{{.TASKFILE}}"
- "lint-requirements.txt"
generates: ["{{.CHECKSUM_FILE}}"]
run: "once"
deps:
- ":init"
- task: ":utils:validate-checksum"
Expand Down
6 changes: 6 additions & 0 deletions src/spider/spider.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#include <iostream>

int main() {
std::cout << "Hello, world!" << std::endl;
return 0;
}