Skip to content

Commit 840c7f4

Browse files
Merge branch 'main' into feature/#77-add-support-for-dependencies.md-like-output
2 parents 983ec3b + cbdf979 commit 840c7f4

File tree

15 files changed

+484
-27
lines changed

15 files changed

+484
-27
lines changed

.github/workflows/checks.yml

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,12 @@ jobs:
6565
run: poetry run nox -s lint:code
6666

6767
- name: Upload Artifacts
68-
uses: actions/upload-artifact@v4.5.0
68+
uses: actions/upload-artifact@v4.6.0
6969
with:
7070
name: lint-python${{ matrix.python-version }}
71-
path: .lint.txt
71+
path: |
72+
.lint.txt
73+
.lint.json
7274
include-hidden-files: true
7375

7476
Type-Check:
@@ -112,7 +114,7 @@ jobs:
112114
run: poetry run nox -s lint:security
113115

114116
- name: Upload Artifacts
115-
uses: actions/upload-artifact@v4.5.0
117+
uses: actions/upload-artifact@v4.6.0
116118
with:
117119
name: security-python${{ matrix.python-version }}
118120
path: .security.json
@@ -157,7 +159,7 @@ jobs:
157159
run: poetry run nox -s test:unit -- -- --coverage
158160

159161
- name: Upload Artifacts
160-
uses: actions/upload-artifact@v4.5.0
162+
uses: actions/upload-artifact@v4.6.0
161163
with:
162164
name: coverage-python${{ matrix.python-version }}-fast
163165
path: .coverage

.github/workflows/report.yml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,10 @@ jobs:
3131
working-directory: ./artifacts
3232
run: |
3333
poetry run coverage combine --keep coverage-python3.9*/.coverage
34+
# Errors during copying are ignored because they are checked in the next step
3435
cp .coverage ../ || true
3536
cp lint-python3.9/.lint.txt ../ || true
37+
cp lint-python3.9/.lint.json ../ || true
3638
cp security-python3.9/.security.json ../ || true
3739
3840
- name: Validate Artifacts
@@ -42,7 +44,7 @@ jobs:
4244
run: poetry run nox -s project:report -- -- --format json | tee metrics.json
4345

4446
- name: Upload Artifacts
45-
uses: actions/upload-artifact@v4.5.0
47+
uses: actions/upload-artifact@v4.6.0
4648
with:
4749
name: metrics.json
4850
path: metrics.json
@@ -54,6 +56,5 @@ jobs:
5456
poetry run nox -s dependency:licenses >> $GITHUB_STEP_SUMMARY
5557
echo -e "\n\n# Coverage\n" >> $GITHUB_STEP_SUMMARY
5658
poetry run coverage report -- --format markdown >> $GITHUB_STEP_SUMMARY
57-
echo -e "\n\n# Static Code Analysis\n" >> $GITHUB_STEP_SUMMARY
58-
cat .lint.txt >> $GITHUB_STEP_SUMMARY
59+
poetry run tbx lint pretty-print >> $GITHUB_STEP_SUMMARY
5960
poetry run tbx security pretty-print .security.json >> $GITHUB_STEP_SUMMARY

.github/workflows/slow-checks.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ jobs:
3939
run: poetry run nox -s test:integration -- -- --coverage --db-version ${{ matrix.exasol-version }}
4040

4141
- name: Upload Artifacts
42-
uses: actions/upload-artifact@v4.5.0
42+
uses: actions/upload-artifact@v4.6.0
4343
with:
4444
name: coverage-python${{ matrix.python-version }}-slow
4545
path: .coverage

doc/changes/unreleased.md

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,24 @@
11
# Unreleased
22

3+
## ✨ Added
4+
5+
* added tbx task for markdown formating of .lint.json
6+
* Added a Nox task for dependencies packages and their licenses with Markdown output
7+
38
## 🐞 Fixed
49
* Fixed an issue in the CI workflow that caused it to be executed twice on the initial push of a PR if the PR branch was on the repo itself.
510

611
🚨 Attention: Due to these changes, the workflows will no longer be executed if the PR comes from a branch not located in this repository.
712
As third-party contributions from outside forks are rare to nearly non-existent, this downside was considered a reasonable trade-off at this time.
8-
13+
14+
## 📚 Documentation
15+
* Updated design doc (Added known Issues)
16+
* Updated migration progress table
17+
* Updated the FAQ with an entry about the ``isort`` compatibility issue
18+
19+
## 🔧 Changed
20+
* Updated `actions/upload-artifacts` version to `4.6.0`
21+
922
## 🔩 Internal
1023
* Relocked dependencies
1124
* Update referenced github actions
12-
13-
## ✨ Added
14-
* Added a Nox task for dependencies packages and their licenses with Markdown output

doc/design.rst

Lines changed: 235 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,3 +209,238 @@ _________________
209209
- Description
210210
* - python-environment
211211
- Sets up an appropriate poetry based python environment
212+
213+
214+
Known Issues
215+
------------
216+
217+
This section documents flaws, sins, and known issues with the current design and/or its current implementation that were either known upfront or surfaced through the course of implementing it. Additionally, it attempts to explain why certain choices were made at the time, so one can better understand whether it may be reasonable to make changes now or in the future.
218+
219+
220+
Passing files as individual arguments on the CLI
221+
++++++++++++++++++++++++++++++++++++++++++++++++
222+
223+
**Description:**
224+
225+
As of today selection of python files for linting, formatting etc. is done by passing all relevant python files as individual argument(s)
226+
to the tools used/invoked by the python toolbox.
227+
228+
**Downsides:**
229+
230+
- Most shells have limitations on the number of arguments and their length.
231+
- Noisy output, making it hard to decipher the actual command.
232+
- Not ideal for all use cases.
233+
234+
**Rationale/History:**
235+
236+
- The current method of passing files as individual arguments by default offers ease in collection and filtering. It also allows users to simply provide or replace the selection mechanism fairly easily.
237+
238+
- Every tool used by the toolbox (e.g., `black`, `isort`) used to support passing files by argument. However, not all of them provided the same mechanism for selection or deselection patterns (e.g. "glob").
239+
240+
**Ideas/Solutions:**
241+
242+
- Develop a wrapper that allows for different selection mechanisms
243+
244+
245+
Inconsistent Naming
246+
+++++++++++++++++++
247+
248+
**Description:**
249+
250+
The naming is not consistent across the project name (python-toolbox) and the PyPI package name (exasol-toolbox).
251+
252+
**Downsides:**
253+
254+
- Misalignment between the PyPI package name and the project name causes confusion when discussing or referring to the project/package.
255+
256+
**Rationale/History:**
257+
258+
- Initially, this was a proof of concept (POC) to verify a few ideas, and the naming was not well thought out at the time.
259+
- Later, when publishing the first package for distribution, the project name was unavailable on PyPI, resulting in a different name being used on PyPI.
260+
261+
**Ideas/Solutions:**
262+
263+
- Consistently rename project to ``exasol-python-toolbox``: `Issue-325 <https://github.com/exasol/python-toolbox/issues/325>`_
264+
265+
Project Configuration
266+
+++++++++++++++++++++
267+
268+
**Description:**
269+
Currently, the documentation regarding the configuration of projects using the toolbox has various gaps and does not follow a clear configuration hierarchy or structure.
270+
271+
**Downsides:**
272+
273+
- Multiple scattered configuration points make management and understanding difficult.
274+
- Configurations overlap or conflict with unclear priorities.
275+
- Tool leakage (e.g., the ``[isort]`` section in ``pyproject.toml``).
276+
(If everything were done via toolbox config file(s), backing tools could be swapped more easily).
277+
278+
**Rationale/History:**
279+
280+
- Initial decisions aimed to simplify individual adjustments in the projects until we had a better understanding of what needed to be configured.
281+
- Scattering configuration across various files and tools was a hasty decision to expedite development and accommodate various tools.
282+
283+
**Ideas/Solutions:**
284+
285+
Currently used methods to configure toolbox-based projects:
286+
287+
#. Project configuration: ``noxconfig.py``
288+
#. Tool-specific configuration files or sections in ``pyproject.toml``
289+
#. Implementing plugin extension points
290+
#. Overwriting nox tasks with custom implementations
291+
#. Replacing with customized workflows of the same name (only applicable for action/workflows)
292+
293+
Refinement:
294+
295+
- Centralize all toolbox based configurations in a toolbox config file (``noxconfig.py``).
296+
- Rename the toolbox config file from ``noxconfig.py`` to a more appropriate name that reflects its purpose.
297+
- Document configuration hierarchy and usage.
298+
299+
300+
Nox Task Runner
301+
+++++++++++++++
302+
303+
**Description:**
304+
While Nox isn't a perfect fit, it still meets most of our requirements for a task runner.
305+
306+
**Downsides:**
307+
308+
- Imports over top-level modules are problematic as all contained tasks are imported.
309+
- Passing and receiving additional arguments to a task is clunky.
310+
- The default behavior of creating a venv for tasks is undesirable.
311+
- Nox does not support grouping.
312+
313+
**Rationale/History:**
314+
315+
Why Nox was choosen:
316+
317+
- No additional language(s) required: There was no need to introduce extra programming languages or binaries, simplifying the development process.
318+
- Python-based: Being Python-based, Nox can be extended and understood by Python developers.
319+
- Python code: As Nox tasks are defined via Python code, existing scripts can be reused and code can be shared easily.
320+
- Simplicity: Nox is relatively "small" in functionality, making it somewhat simple to use and understand.
321+
322+
**Ideas/Solutions:**
323+
324+
Grouping:
325+
326+
Since Nox doesn't natively support task grouping, we need a strategy to group commands.
327+
Therefore, a naming convention to indicate grouping should be adopted.
328+
329+
Suggestion: Groups will be separated using a :code:`:` (colon) because :code:`-` (dash) might already be used within task names.
330+
331+
Imports:
332+
333+
Consider modularizing tasks to handle top-level imports better.
334+
335+
Others Issues:
336+
337+
Generally, one may consider addressing the other issues by choosing another task runner or creating a small set of CLI tools and extension points manually provided by the toolbox.
338+
339+
340+
Poetry for Project Management
341+
+++++++++++++++++++++++++++++
342+
343+
While poetry was and is a good choice for Exasol project, dependency, build tool etc. "most recently"
344+
`uv <https://docs.astral.sh/uv/>`_ has surfaced and made big advanced. Looking at uv it addresses additional itches with
345+
our projects and therefore in the long run it may be a good idea to migrate our project setups to it.
346+
Use poetry for project, build and dependency management.
347+
348+
349+
Code Formatting
350+
+++++++++++++++
351+
352+
**Description:**
353+
354+
Currently we use Black and Isort for code formatting, though running them on a larger code base as pre-commit hooks or such can take quite a bit of time.
355+
356+
**Downsides:**
357+
358+
- Two tools and an aligned configuration of them are required to cleanly and correctly format the codebase.
359+
- Code needs to be processed at least twice as we apply two individual tools.
360+
- The performance of Black and Isort is okay but not great compared to other tools.
361+
362+
**Rationale/History:**
363+
364+
- Black and Isort have been used because they are battle-tested and widely used
365+
- When we opted for Black and Isort, ``ruff`` wasn't "a thing" yet and at best in its early stages.
366+
- Black and Isort already have been known by most python devs when we where selecting the tools
367+
368+
**Ideas/Solutions:**
369+
370+
As `Ruff <https://docs.astral.sh/ruff/>`_ is fairly stable and also tested and used by many Python projects
371+
we should consider transitioning to it.
372+
373+
Advantages:
374+
375+
- Well-tested
376+
- Widely used
377+
- Excellent performance
378+
- Single tool for imports and formatting the codebase
379+
- Simplifies adopting ruff for linting
380+
381+
382+
Pylint
383+
++++++
384+
385+
**Description:**
386+
We are currently using Pylint instead of Ruff.
387+
388+
**Downsides:**
389+
390+
- Pylint is slower and less usable in pre-commit hooks
391+
- It is an additional tool, therefore at least one more processing run of the code is required
392+
- No support for Language Server Protocol (LSP, e.g. compare to `ruff lsp`)
393+
394+
**Rationale/History:**
395+
396+
- Well known
397+
- Pylint provides built-in project score/rating
398+
- Project score is good for improving legacy code bases which haven't been linted previously
399+
- Plugin support
400+
401+
**Ideas/Possible Solutions:**
402+
403+
Replacing Pylint with Ruff for linting would provide significant performance improvement. Additionally, Ruff offers an LSP and IDE integrations and is widely used these days. Additionaly there would be an additional synergy if we adopt ruff for formatting the code base.
404+
405+
Transitioning to Ruff requires us to adjust the migration and improvement strategies for our projects:
406+
407+
- Currently, our codebase improvements are guided by scores. However, with Ruff, a new approach is necessary. For example, we could incrementally introduce specific linting rules, fix the related issues, and then enforce these rules.
408+
409+
- The project rating and scoring system will also need modification. One possiblity would be to run Ruff and Pylint in parallel, utilizing Pylint solely for rating and issue resolution while Ruff is incorporated for linting tasks.
410+
411+
412+
Security Linter
413+
+++++++++++++++
414+
415+
**Description:**
416+
As of today, the security linter does not fail if it has findings. This was intentionally done to simplify integration and adoption of the tool. Developers can still use the results to improve and find issues within the codebase, and additionally, a rating will be generated to provide some guidance on which projects need attention.
417+
418+
**Downsides:**
419+
- No enforced safeguard on introducing potential security issues
420+
421+
**Rationale/History:**
422+
- Simplify adoption into projects
423+
- First step to introduce tooling and make the current state/rating visible
424+
425+
**Ideas/Possible Solutions:**
426+
* Define a strategy to address potential security issues in projects. Once this has been done, enforce the immediate addressing of potential security issues in the codebase upon introduction.
427+
* Allow excluding individual findings in projects until they are fixed.
428+
429+
430+
Workflows Dependency Structure
431+
++++++++++++++++++++++++++++++
432+
433+
**Description:**
434+
Undocumented workflow interdependencies and structure
435+
436+
**Downsides:**
437+
- Hard to customize if one does not understand the overall setup and dependencies
438+
439+
**Rationale/History:**
440+
- Simplify development during the discovery phase (what is needed, how to implement, adjust to discovered needs)
441+
- Ideally, all workflows will be integrated and use a standard setup (part of the customization can also be done in the called nox tasks)
442+
443+
**Ideas/Possible Solutions:**
444+
445+
- Define clear requirements and interfaces
446+
- Document those requirements and interfaces

doc/faq.rst

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,3 +63,33 @@ Limited Previous Versions in Multiversion Documentation
6363
-------------------------------------------------------
6464

6565
If not all previous versions of the project are available via the version selection box of the multiversion documentation, it is likely due to the fact that the unavailable documentation for those versions was not in a compatible format (there hasn't been a compatible setup of a Sphinx-based documentation).
66+
67+
68+
.. _faq_failing_format_check:
69+
70+
Format Still Fails After Running ``project:fix``
71+
------------------------------------------------
72+
73+
If running the following sequence of commands results in ``project:format`` failing with an error during the execution of ``isort``:
74+
75+
#. Run ``project:fix``
76+
#. Run ``project:format``
77+
78+
It is very likely that you did not configure ``isort`` and/or ``black`` appropriately in your ``pyproject.toml`` file.
79+
80+
Ensure ``isort`` is configured with compatibility for ``black``:
81+
82+
.. code-block:: toml
83+
84+
[tool.isort]
85+
profile = "black"
86+
force_grid_wrap = 2
87+
88+
Additionally, your black configuration should look similar to this:
89+
90+
.. code-block:: toml
91+
92+
[tool.black]
93+
line-length = 88
94+
verbose = false
95+
include = "\\.pyi?$"

doc/user_guide/migrating.rst

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,10 +199,10 @@ Could be tracked in a format and based on the information listed in the real lif
199199
-
200200
-
201201
-
202-
-
203202
-
204-
-
205-
-
203+
-
204+
-
205+
-
206206
* - `sqlalchemy-exasol <https://github.com/exasol/sqlalchemy-exasol>`_
207207
-
208208
-

exasol/toolbox/nox/_artifacts.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ def check_artifacts(session: Session) -> None:
2323
error = False
2424
if msg := _validate_lint_txt(Path(PROJECT_CONFIG.root, ".lint.txt")):
2525
print(f"error in [.lint.txt]: {msg}")
26+
if msg := _validate_lint_json(Path(PROJECT_CONFIG.root, ".lint.json")):
27+
print(f"error in [.lint.json]: {msg}")
2628
if msg := _validate_security_json(Path(PROJECT_CONFIG.root, ".security.json")):
2729
print(f"error in [.security.json]: {msg}")
2830
error = True

0 commit comments

Comments
 (0)