sourcery-ai / sourcery

Instant AI code reviews
https://sourcery.ai
MIT License
1.5k stars 65 forks source link

mapping values are not allowed here #409

Closed stsmrvestas closed 5 months ago

stsmrvestas commented 6 months ago

Checklist

Description

I'm new, so apologies if this is a user error, however I'm getting this error from a pattern, which looks almost identical to what you have in your docs afaict. It is complaining when attempting review from CLI:

.sourcery.yaml:41:27-41:28 (root) mapping values are not allowed here
in .sourcery.yaml", line 42, column 28:
        pattern: class ${klass}:
                               ^ (line: 42)

Code snippet that reproduces issue

<cut>
rules:
  - id: service-class-naming
    description: Service class naming convention
    pattern: class ${klass}:
    condition: not klass.matches_regex("^[A-Z][a-zA-Z0-9]*Service(\(.*|):$")
    explanation: 'Service classes must be postfixed with "Service" - if you have additional classes in the same file, please move them to a separate file'

Debug Information

IDE Version: PyCharm 2023.3.1 (Professional Edition)

Sourcery Version: sourcery, version 1.15.0

Operating system and Version: OSX Sonoma 14.2.1 (23C71)

ruancomelli commented 5 months ago

Hello, @stsmrvestas! Thanks for reaching out!

The error message you are seeing here actually comes from our YAML parser. Because of the YAML syntax, our parser (correctly) interprets the line pattern: class ${klass}: as if you were building a nested dictionary {"pattern": {"class ${klass}": ""}}. Strange, right?

To fix this, you can use a multiline YAML string:

rules:
  - id: service-class-naming
    description: Service class naming convention
    pattern: |
      class ${klass}(...):
        ...
    condition: not klass.ends_with("Service")
    explanation: 'Service classes must be postfixed with "Service" - if you have additional classes in the same file, please move them to a separate file'

I have also fixed the Sourcery pattern syntax for you by using the universal matcher ... and simplified the matching condition by using the ends_with conditional, which (I think?) is what your rule is about.

Please let me know if this does (or does not) work for you.

stsmrvestas commented 5 months ago

Thanks - that works. I think I was thrown by a couple of things:

  1. I need to include the '...' aka class body - it is not sufficient to simply do a single line even if I don't care about the body of the class - that's prolly how it has to be, but not 100% intuitive..
  2. the tests also needs to follow the same rule (class body required)
  3. Your suggestion doesn't take into appear to take into account when the classes are like this:
    class SomeClass:  # no parenthesis
    ...

    but even with your proposal, the above example actually works and detects that SomeClass does not end with Service even though it doesn't strictly match the 'class ${klass}(...):' pattern which has the parenthesis. This last thing is what I want, but it is not clear why it actually works, and I would never have guess this?

Anyways thanks for the assistance!