configurable terms for todo-in-comment

[rikerfi]

It would be good if todo-in-comment rule has configurable terms.

E.g. robocop --configure todo-in-comment:todos:OMG,bug also this work robocop --configure "todo-in-comment:todos:remove me,fix this ugly bug" This should enable also terms in local language.

Work in progress: documentation, tests

What would be the best location to place rule configuration tests?

[bhirsz]

Like you probably noticed our 'test framework' auto generates test for all rules with default configs. To test rule with custom config we can use following file:

https://github.com/MarketSquare/robotframework-robocop/blob/master/tests/atest/custom_tests.yaml

Either add entry with name of the rule / or update if the rule is already there with your desired config and path to test data files.

For example:

  inconsistent-assignment:
    - config: inconsistent-assignment:assignment_sign_type:space_and_equal_sign
      src_dir: misc/inconsistent-assignment-const
    - config: inconsistent-assignment:assignment_sign_type:autodetect
      src_dir: misc/inconsistent-assignment-autodetect

Runs two separate tests for rule inconsistent-assignment with different config and test data directories.

This:

  not-allowed-char-in-filename:
    - config: not-allowed-char-in-filename:pattern:\.(?!bar)
      src_dir: naming/not-allowed-char-in-filename-configured

is equivalent of running (in the tests/atest directory):

robocop --include not-allowed-char-in-filename --configure  not-allowed-char-in-filename:pattern:\.(?!bar) naming/not-allowed-char-in-filename-configured

Config can be single string (if you're only configuring one param) or list of strings (after #672 will be merged). But I think you don't need to configure more than one param per test so you don't need to wait for 672 and just use string for configuration.

[bhirsz]

I actually put this information in our CONTRIBUTING docs but it's less detailed here, maybe I should update it soon ;) This can get confusing - our test framework is useful for our needs but even I (author) tend to forgot how it works with autogeneration/custom tests.

Edit - I did not put this information in contributing docs. Uh!

[codecov-commenter]

Codecov Report

Merging #674 (9fef356) into master (41943b0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #674   +/-   ##
=======================================
  Coverage   97.40%   97.40%           
=======================================
  Files          23       23           
  Lines        2886     2893    +7     
=======================================
+ Hits         2811     2818    +7     
  Misses         75       75           

Impacted Files	Coverage Δ
robocop/checkers/comments.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[rikerfi]

Something hassle with WIN environment. I try to fix that by guessing the problem is with Unicode characters. Unfortunately, I don't have WIN device available anytime soon. I need to postpone the PR a few days to find one 😓

[bhirsz]

Something hassle with WIN environment. I try to fix that by guessing the problem is with Unicode characters. Unfortunately, I don't have WIN device available anytime soon. I need to postpone the PR a few days to find one 😓

I can try to run in - I will let you know. From the results I also see it's running some of the tests that should be disabled by default (like benchmark test). It's most likely because of the 'tox -> nox' migration that happened recently. I will fix that one too

[bhirsz]

Something hassle with WIN environment. I try to fix that by guessing the problem is with Unicode characters. Unfortunately, I don't have WIN device available anytime soon. I need to postpone the PR a few days to find one 😓

I can try to run in - I will let you know. From the results I also see it's running some of the tests that should be disabled by default (like benchmark test). It's most likely because of the 'tox -> nox' migration that happened recently. I will fix that one too

There is problem with encoding, but it's not connected to your PR. All values passed to arguments/options are affected by it. I need to think about solution - it's not easy mix of argparse & encoding for specific platform

[bhirsz]

Something hassle with WIN environment. I try to fix that by guessing the problem is with Unicode characters. Unfortunately, I don't have WIN device available anytime soon. I need to postpone the PR a few days to find one 😓

I can try to run in - I will let you know. From the results I also see it's running some of the tests that should be disabled by default (like benchmark test). It's most likely because of the 'tox -> nox' migration that happened recently. I will fix that one too

There is problem with encoding, but it's not connected to your PR. All values passed to arguments/options are affected by it. I need to think about solution - it's not easy mix of argparse & encoding for specific platform

Another update - it might be also issue in how I read the test data. There is hope!

[rikerfi]

Something hassle with WIN environment. I try to fix that by guessing the problem is with Unicode characters. Unfortunately, I don't have WIN device available anytime soon. I need to postpone the PR a few days to find one sweat

I can try to run in - I will let you know. From the results I also see it's running some of the tests that should be disabled by default (like benchmark test). It's most likely because of the 'tox -> nox' migration that happened recently. I will fix that one too

There is problem with encoding, but it's not connected to your PR. All values passed to arguments/options are affected by it. I need to think about solution - it's not easy mix of argparse & encoding for specific platform

Ok, thanks for checking. I'll rollback the latest commit to keep PR simple.

[bhirsz]

Ups, for some reason the PR auto closed itself - working on the fix.

[rikerfi]

Ups, for some reason the PR auto closed itself - working on the fix.

No worries, I could create new one.

[bhirsz]

Ups, for some reason the PR auto closed itself - working on the fix.

I can't do it easily due to missing permissions (forked repo). Can you revert this PR somehow>? If not I can investigate more.

To get it working on windows you need to add encoding="utf-8" here:

robotframework-robocop/tests/atest/conftest.py

Line 49 in 111b1f7

with open(Path(__file__).parent / "custom_tests.yaml") as f:

and here:

robotframework-robocop/tests/atest/test_rule.py

Line 45 in 111b1f7

with open(expected_file) as f:

[bhirsz]

Looks good now! I will leave few comments regarding lower/upper case, it's not strictly connected to your changed but my though in general for this rule

[mnojek]

Great addition! Thanks, @rikerfi ! And sorry for being so nit-picky! 😉

[rikerfi]

Great addition! Thanks, @rikerfi ! And sorry for being so nit-picky! wink

Hah, no worries. Reviews are the right place for that 😄