Skip to content

SCANPY-156 Expand documentation for the pysonar-scanner#186

Merged
Seppli11 merged 4 commits intomasterfrom
SCANPY-156
Apr 2, 2025
Merged

SCANPY-156 Expand documentation for the pysonar-scanner#186
Seppli11 merged 4 commits intomasterfrom
SCANPY-156

Conversation

@guillaume-dequenne
Copy link
Copy Markdown
Contributor

@guillaume-dequenne guillaume-dequenne commented Apr 2, 2025

@guillaume-dequenne guillaume-dequenne force-pushed the SCANPY-156 branch 4 times, most recently from da8768f to 2015180 Compare April 2, 2025 09:47
@guillaume-dequenne guillaume-dequenne changed the title SCANPY-156 SCANPY-156 Expand documentation for the pysonar-scanner Apr 2, 2025
@guillaume-dequenne guillaume-dequenne force-pushed the SCANPY-156 branch 9 times, most recently from b148d21 to bd673ea Compare April 2, 2025 12:37
@guillaume-dequenne guillaume-dequenne marked this pull request as ready for review April 2, 2025 13:10
Copy link
Copy Markdown
Contributor

@Seppli11 Seppli11 left a comment

Choose a reason for hiding this comment

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

Overall, this looks really good.
I really like the approach with the CLI_ARGS.md since it guarantees that it is always up to date.

I've left a few comments. Some personal preferences and remarks, others corrections.

Comment thread README.md Outdated

- SonarQube v9.9 or higher
- SonarQube v10.6 or above
- Python 3.8 or above
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.

We require python 3.9

Suggested change
- Python 3.8 or above
- Python 3.9 or above

Comment thread README.md Outdated
When a `pyproject.toml` file is available, it is possible to set the `-read-project-config` flag
to allow the scanner to deduce analysis properties from the project configuration.
```
pysonar --sonar-projectBaseDir "path/to/projectBaseDir"
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.

I think you meant -Dsonar.projectBaseDir here, since --sonar-projectBaseDir doesn't exist.

Suggested change
pysonar --sonar-projectBaseDir "path/to/projectBaseDir"
pysonar -Dsonar.projectBaseDir="path/to/projectBaseDir"

Comment thread .cirrus.yml
- poetry run licenseheaders -t license_header.tmpl -o "SonarSource SA" -y 2011-2024 -n "Sonar Scanner Python" -E .py -d tests/
- git diff --name-only --exit-code ./src ./tests

documentation_task:
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.

awesome 💪

Comment thread README.md
Analysis properties can be provided as CLI arguments to the `pysonar` command.
They follow the same convention as when running the SonarScanner CLI directly
(see [documentation](https://docs.sonarsource.com/sonarqube/9.9/analyzing-source-code/scanners/sonarscanner/#running-from-zip-file)).
They can be provided in a similar way as when running the SonarScanner CLI directly
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.

Personally, I would have formulated this the other way around. E.g. use --token and --sonar-region by default. If the argument is not available, -D... works for everything, but we don't verify and blindly forward it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I intentionally framed it that way so that the first information is the one that will definitely work, then only later we provide a way to make it less verbose for specific properties.
I feel that reduces the risk of mistakes if someone only skims the documentation. Does it make sense?

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.

Yeah, I think so.

Comment thread tools/generate_cli_documentation.py Outdated

# Group arguments by category
categories = {
"Authentication": ["token", "sonar_host_url", "sonar_region"],
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.

we probably should also add sonar_organization here (depending if #188 is merged first)

Comment thread CLI_ARGS.md
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.

The result looks good!

Comment thread README.md
Comment on lines +26 to +31
1. Through CLI arguments to the `pysonar` command
2. Environment variables for individual properties (e.g. `SONAR_TOKEN`, `SONAR_VERBOSE`, `SONAR_HOST_URL`, ...)
3. Generic environment variable `SONAR_SCANNER_JSON_PARAMS`
4. Under the `[tool.sonar]` key of the `pyproject.toml` file
5. In a dedicated `sonar-project.properties` file
6. Through common properties extracted from the `pyproject.toml`
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.

<3

Comment thread README.md

You can use all the argument allowed by __SonarScanner__.
For more information on __SonarScanner__ please refer to the [SonarScanner documentation](https://docs.sonarsource.com/sonarqube/9.9/analyzing-source-code/scanners/sonarscanner/)
You can use all the arguments allowed by __SonarScanner__.
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.

It could be worth it to mention that we support also Python specific properties like sonar.python.version

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm tempted to think it's implied, since CLI_ARGS.md mentions them explicitly.
We could keep a list of Python specific properties documented separately though, but I feel it might be overkill at this point?

"flake8_report_paths",
"ruff_report_paths",
],
"Other": [], # Will contain everything else
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.

I'd like to see a Python specific section with :
--sonar-cpd-python-minimum-lines
--sonar-cpd-python-minimum-tokens
--sonar-python-skip-unchanged
--toml-path
--sonar-python-skip-unchanged
--sonar-python-xunit-skip-details
--sonar-python-version

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As discussed, it probably make sense to have amore granular split of the "Other" category, but I'm not sure doing it through Python-exclusive section makes sense for the end user (they probably care more about what the property achieve rather than the fact it's specific to the Python analyzer). As a baby step, let's keep it like this and revisit later.

@sonarqube-next
Copy link
Copy Markdown

sonarqube-next bot commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

@thomas-serre-sonarsource thomas-serre-sonarsource left a comment

Choose a reason for hiding this comment

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

I like the idea !
I left a few subjective comments :)

Copy link
Copy Markdown
Contributor

@Seppli11 Seppli11 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the quick changes

@Seppli11 Seppli11 merged commit 49e7c9f into master Apr 2, 2025
18 checks passed
@Seppli11 Seppli11 deleted the SCANPY-156 branch April 2, 2025 14:25
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.

3 participants