Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the possibility to show the default values for parameters #135

Conversation

sebastiaanspeck
Copy link
Contributor

Also improved the formatting a little:

Before:

- unused-variable (Variable is unused)
        unused-variable.check-parameters:       true (Check for unused parameters)

After:

- unused-variable (Variable is unused)
  * check-parameters: true (Check for unused parameters)

Adding --with-default-values to --show-checks, generates the next output:

Enabled checks:
- commented-code (Remove commented code)
  * (default) min-lines: 3 (Minimum number of commented lines)
- duplicate-method-in-file (Duplicate method definition in file)
- empty-block (Block is empty and can be removed)
- exemplar-slot-count (Exemplar has too many slots)
  * (default) max-slot-count: 10 (Maximum number of slots for an exemplar)
- file-method-count (File has too many methods defined)
  * (default) max-method-count: 10 (Maximum number of methods in file)
- file-not-in-load-list (File is not included in load_list)
- forbidden-call (Forbidden call)
  * (default) forbidden-calls (List of forbidden calls, separated by ','):
    - show()
    - sw:show()
    - print()
    - sw:print()
    - debug_print()
    - sw:debug_print()
    - .sys!perform()
    - .sys!slot()
- forbidden-global-usage (Forbidden usage of global)
  * (default) forbidden-globals (List of forbidden globals, separated by ','):
    - !current_grs!
    - sw:!current_grs!
- forbidden-inheritance (Forbidden inheritance)
  * (default) forbidden-parents:  (Forbidden parents to inhertit from, separated by ',')
- formatting (Improper formatting)
  * (default) indent-character: tab (The character used for indentation (tab/space))
  * (default) tab-width: 8 (The width of a tab character)
- hides-variable (Variable definition hides another variable with the same name.)
- import-missing-definition (Import missing definition)
- lhs-rhs-comparator-equal (Left hand side and right hand side of comparison are the same)
- line-length (Lines should not be too long)
  * (default) max-line-length: 120 (Maximum number of characters on a single line)
  * (default) tab-width: 8 (The width of a tab character)
- local-import-procedure (Possibly meant _import instead of _local)
- method-complexity (Methods should not be too complex)
  * (default) max-complexity: 10 (Maximum complexity of method by the McCabe definition)
- method-line-count (Method/procedure is too long)
  * (default) max-length: 35 (Maximum length of method in lines without white lines and comment lines)
- nesting-depth (Methods/procedures/if-statements/loop-statements should not exceed the maximum nesting depth)
  * (default) max-nesting-depth: 3 (Maximum nesting depth)
  * (default) count-early-return-as-nesting-depth: true (Count early return as nesting depth)
- no-self-use (No usage of _self)
- no-statement-after-body-exit (No statement after body exit)
- parameter-count (Methods/procedures should not have too many parameters)
  * (default) max-parameter-count: 6 (Maximum number of parameters for a method or procedure)
- scope-count (Too many variables in scope)
  * (default) max-scope-count: 25 (Maximum number of entries in scope)
- simplify-if (If statement can be simplified)
- size-zero-empty (Use .empty? instead of .size = 0)
- sw-method-doc (Method is missing documentation in Smallworld style)
  * (default) allow-blank-method-doc: false (Allow blank method doc)
- syntax-error (Syntax error)
- todo-comment (TODO comment)
  * (default) forbidden-comment-words (List of forbidden words, separated by ',', case sensitive):
    - TODO
    - FIXME
    - HACK
    - NOTE
    - TEMP
    - XXX
- trailing-whitespace (Remove the trailing whitespace)
- type-doc (Method is missing documentation)
- undefined-variable (Prefixed variable used as a global)
- unsafe-evaluate-invocation (Use of unsafe_evaluate())
- unused-variable (Variable is unused)
  * check-parameters: true (Check for unused parameters)
- use-value-compare (Compare strings and bignums with the equality-operators)
- variable-count (Method/procedure contains too many variables)
  * (default) max-variable-count: 8 (Maximum number of variables in method)
- variable-declaration-usage-distance (Distance between variable definition and usage)
  * (default) max-distance: 5 (Maximum distance between declaration and usage)
  * (default) ignore-constants: true (Ignore declared constants)
- variable-naming (Give variables a proper descriptive name)
  * (default) max-length: 32 (Maximum number of characters for a variable name)
  * (default) min-length: 3 (Minimum number of characters for a variable name)
  * (default) whitelist:  (Whitelist (comma separated) of variable names to allow/ignore.)
- warned-call (Warned call)
  * (default) warned-calls (List of Warned calls, separated by ','):
    - write()
    - sw:write()
    - remex()
    - sw:remex()
    - remove_exemplar()
    - sw:remove_exemplar()
Disabled checks:

@sebastiaanspeck sebastiaanspeck marked this pull request as draft November 16, 2024 15:39
@sebastiaanspeck sebastiaanspeck marked this pull request as ready for review November 16, 2024 15:56
@sebastiaanspeck sebastiaanspeck force-pushed the feature/more-extensive-check-showing-behaviour branch from bee6ed7 to d921670 Compare November 16, 2024 15:57
Copy link
Owner

@StevenLooman StevenLooman left a comment

Choose a reason for hiding this comment

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

- unused-variable (Variable is unused)
  * check-parameters: true (Check for unused parameters)

Please keep the full parameter name. This makes it less confusing when configuring your magik-lint.properties. Now one might think the key to set is check-parameters, instead of unused-variable.check-parameters

- forbidden-call (Forbidden call)
  * (default) forbidden-calls (List of forbidden calls, separated by ','):
    - show()
    - sw:show()
    - print()
    - sw:print()
    - debug_print()
    - sw:debug_print()
    - .sys!perform()
    - .sys!slot()

I understand the splitting/generating the list. But when configuring your magik-lint.properties it does expect a single string. This causes confusion.

I'm not a fan of this. It makes the software more complex and I doubt it adds that much value. It might be better to add the defaults to the wiki, through automation of some sort, if we would really want this. You can already see the defaults when running the linter(s) without a config file.

* Show parameters for a check.
*
* @param writer Writer Write to write output to.
* @param showDefaultValues boolean Boolean to show default values for parameters or not.
Copy link
Owner

Choose a reason for hiding this comment

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

No need to state that it is a boolean. Java includes types for their parameters etc. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was copied from the method above (showChecks), but I probably introduced that as well.

final Writer writer, final MagikCheckHolder holder, final boolean showDefaultValues)
throws ReflectiveOperationException, IOException {
for (final MagikCheckHolder.Parameter parameter : holder.getParameters()) {
if (!parameter.isExplicitlySet() && !showDefaultValues) {
Copy link
Owner

Choose a reason for hiding this comment

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

So if no value is set, and the --with-default-values is not given, the parameter will not be shown? That seems a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the behaviour before this PR as well. Only user-changed parameters were shown. I kept that behaviour, but if a user uses --with-default-values, the default values will be shown as well.

@@ -50,6 +50,11 @@ public final class Main {
.build();
private static final Option OPTION_SHOW_CHECKS =
Option.builder().longOpt("show-checks").desc("Show checks and exit").build();
private static final Option OPTION_WITH_DEFAULT_VALUES =
Copy link
Owner

Choose a reason for hiding this comment

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

A better name would be --show-check-parameters-default-values or something.

@sebastiaanspeck sebastiaanspeck marked this pull request as draft November 18, 2024 03:09
@krn-sebastiaan krn-sebastiaan force-pushed the feature/more-extensive-check-showing-behaviour branch from 582238a to 283abae Compare November 18, 2024 07:18
@krn-sebastiaan krn-sebastiaan force-pushed the feature/more-extensive-check-showing-behaviour branch from 283abae to 130e71c Compare November 18, 2024 07:19
@sebastiaanspeck sebastiaanspeck marked this pull request as ready for review November 18, 2024 10:44
@sebastiaanspeck sebastiaanspeck marked this pull request as draft November 18, 2024 10:44
dependabot bot and others added 3 commits November 18, 2024 11:48
)

Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/davidanson/markdownlint-cli2-action) from 17 to 18.
- [Release notes](https://github.com/davidanson/markdownlint-cli2-action/releases)
- [Commits](DavidAnson/markdownlint-cli2-action@v17...v18)

---
updated-dependencies:
- dependency-name: DavidAnson/markdownlint-cli2-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@sebastiaanspeck sebastiaanspeck deleted the feature/more-extensive-check-showing-behaviour branch November 20, 2024 21:03
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.

2 participants