Skip to content

Conversation

mjohanse-emr
Copy link
Contributor

@mjohanse-emr mjohanse-emr commented Sep 8, 2025

Fixes #233

Update the styleguide to check the following fields in descending priority order to find the application name.

  • tool.ni-python-styleguide.application-import-names
  • project.import-names
  • project.name
  • tool.poetry.name

I created some unit tests for _get_application_import_names()

Signed-off-by: Michael Johansen <michael.johansen@ni.com>
Copy link
Contributor

github-actions bot commented Sep 8, 2025

Thank you for contributing! 👋

@mjohanse-emr mjohanse-emr marked this pull request as ready for review September 8, 2025 20:17
Signed-off-by: Michael Johansen <michael.johansen@ni.com>
@mjohanse-emr mjohanse-emr requested a review from bkeryan September 9, 2025 14:44
@mshafer-NI
Copy link
Collaborator

I did not add or update any unit tests for this change. Please let me know if that is required and point me to where I need to add the tests.

We don't currently have tests that cover this...

I wonder if the right way to do this would be to add some example pyproject.toml files into the

test_fix.py example folders that set or don't set this value with a name ("foo"? ) so that's it's part of the testing that the name gets treated as third-party if not set, but firstparty if set.

@mjohanse-emr
Copy link
Contributor Author

I did not add or update any unit tests for this change. Please let me know if that is required and point me to where I need to add the tests.

We don't currently have tests that cover this...

I wonder if the right way to do this would be to add some example pyproject.toml files into the

test_fix.py example folders that set or don't set this value with a name ("foo"? ) so that's it's part of the testing that the name gets treated as third-party if not set, but firstparty if set.

Are those snapshot folders actually pytest snapshots? It might not be a good idea to put loose files in those folders since they are maintained by running pytest with some sort of update_snapshot flag.

@bkeryan
Copy link
Contributor

bkeryan commented Sep 9, 2025

@mjohanse-emr @mshafer-NI Does this feature really need end-to-end CLI testing? How about writing unit tests for the _get_application_import_names() function? I think that would be a lot easier to write and maintain.

Also, as an alternative to saving multiple pyproject.tomls as test assets, you could write setup code that copies the root pyproject.toml and use tomlkit to modify it, or generate one on the fly, or use toml embedded as multi-line strings. That would prevent tooling such as Renovate from seeing a bunch of pyproject.toml files.

Signed-off-by: Michael Johansen <michael.johansen@ni.com>
@mjohanse-emr
Copy link
Contributor Author

@mjohanse-emr @mshafer-NI Does this feature really need end-to-end CLI testing? How about writing unit tests for the _get_application_import_names() function? I think that would be a lot easier to write and maintain.

Also, as an alternative to saving multiple pyproject.tomls as test assets, you could write setup code that copies the root pyproject.toml and use tomlkit to modify it, or generate one on the fly, or use toml embedded as multi-line strings. That would prevent tooling such as Renovate from seeing a bunch of pyproject.toml files.

I added some unit tests that create a "pyproject object" that is just a dictionary and run _get_application_import_names on it.

Signed-off-by: Michael Johansen <michael.johansen@ni.com>
@mshafer-NI mshafer-NI merged commit 2f35231 into ni:main Sep 15, 2025
19 checks passed
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.

application-import-names should fall back to project.name if tool.poetry.name is not specified

4 participants