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

Use a set when calculating resolve names #18939

Merged
merged 1 commit into from
May 8, 2023

Conversation

thejcannon
Copy link
Member

I saw errors with duplicates, so using a set (which is OK since this gets sorted) to stop that.

@thejcannon thejcannon added the category:bugfix Bug fixes for released features label May 8, 2023
@thejcannon thejcannon requested review from stuhood and benjyw May 8, 2023 18:52
@kaos
Copy link
Member

kaos commented May 8, 2023

Is it not unexpected to get duplicates here?

Would be bad to hide a configuration error.. rather than giving an error (or at least a warning)

Do you have an example for what scenario you have where duplicates are a valid thing to have?

@kaos
Copy link
Member

kaos commented May 8, 2023

Ah, overlooked this was for the error message only. Nvm me then :)

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

I still wonder why you'd get duplicates though, and can you export them properly still for those that have dupes?

@thejcannon
Copy link
Member Author

So I did --resolve=foo and saw (among others) pytest and flake8 twice. I was able to export flake8 just fine though...

@kaos
Copy link
Member

kaos commented May 8, 2023

Feels like there might be something registered twice then... perhaps 🤷🏽 👀

@kaos
Copy link
Member

kaos commented May 8, 2023

as such, that feels like a bug we want to squash rather than bury. I'll dig a little.

@kaos
Copy link
Member

kaos commented May 8, 2023

OK- now I see why it gets duplicated, and it's all right by design.

As we list all user resolves along with all tool resolves, and in case you have a custom lockfile for a tool, that gets duplicate entries in this list. So all good. 👍🏽

@kaos
Copy link
Member

kaos commented May 8, 2023

It was reassuring to see there was no duplicate entries registered, which helped guide me to look in the proper spot (which ironically was in the PR diff context already)

❯ pants GenerateToolLockfileSentinel --help-advanced

`pants.core.goals.generate_lockfiles.GenerateToolLockfileSentinel` api type
---------------------------------------------------------------------------

Tools use this as an entry point to say how to generate their tool lockfile.

Each language ecosystem should set up a union member of `GenerateLockfile`, like
`GeneratePythonLockfile`, as explained in that class's docstring.

Each language ecosystem should also subclass `GenerateToolLockfileSentinel`, e.g.
`GeneratePythonToolLockfileSentinel`. The subclass does not need to do anything - it is only used to know which language ecosystems tools correspond to.

Then, each tool should subclass their language ecosystem's subclass of `GenerateToolLockfileSentinel` and set up a rule that goes from the
subclass -> the language's lockfile request, e.g. BlackLockfileSentinel ->
GeneratePythonLockfile. Register `UnionRule(GenerateToolLockfileSentinel, MySubclass)`.

activated by       : pants.core
union members      : AutoflakeLockfileSentinel
                     BlackLockfileSentinel
                     CoverageSubsystemLockfileSentinel
                     DebugPyLockfileSentinel
                     DocformatterLockfileSentinel
                     DockerfileParserLockfileSentinel
                     Flake8LockfileSentinel
                     GoogleJavaFormatToolLockfileSentinel
                     IPythonLockfileSentinel
                     IsortLockfileSentinel
                     JarJarGeneratorLockfileSentinel
                     JarToolGenerateLockfileSentinel
                     JavaParserToolLockfileSentinel
                     JunitToolLockfileSentinel
                     MyPyExtraTypeStubsLockfileSentinel
                     MyPyLockfileSentinel
                     PyOxidizerLockfileSentinel
                     PytestLockfileSentinel
                     ScalaParserToolLockfileSentinel
                     ScalafmtToolLockfileSentinel
                     ScalatestToolLockfileSentinel
                     SetuptoolsLockfileSentinel
                     SetuptoolsSCMLockfileSentinel
                     StripJarToolLockfileSentinel
                     TwineSubsystemLockfileSentinel
                     YapfLockfileSentinel
dependencies       :
dependents         : pants.backend.docker
                     pants.backend.experimental.java
                     pants.backend.experimental.java.lint.google_java_format
                     pants.backend.experimental.python
                     pants.backend.experimental.python.packaging.pyoxidizer
                     pants.backend.experimental.scala
                     pants.backend.experimental.scala.lint.scalafmt
                     pants.backend.python
                     pants.backend.python.lint.autoflake
                     pants.backend.python.lint.black
                     pants.backend.python.lint.docformatter
                     pants.backend.python.lint.flake8
                     pants.backend.python.lint.isort
                     pants.backend.python.typecheck.mypy
                     pants.core
returned by 0 rules:
consumed by 0 rules:
used in 1 rule     : pants.core.goals.generate_lockfiles.generate_lockfiles_goal

@thejcannon
Copy link
Member Author

Thanks for doing the homework <3

@thejcannon thejcannon merged commit b1774d9 into pantsbuild:main May 8, 2023
@thejcannon thejcannon deleted the useset branch May 8, 2023 20:27
thejcannon added a commit to thejcannon/pants that referenced this pull request May 8, 2023
I saw errors with duplicates, so using a `set` (which is OK since this
gets `sorted`) to stop that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants