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

Allow paths for top namespace #34

Merged
merged 6 commits into from
Jun 2, 2023

Conversation

ryaminal
Copy link
Contributor

Description

Adding the forward slash / to the filter for top-namespace when normalizing

Motivation and Context

We would like to create a top level with something like foo/bar so that the module can be imported as import foo.bar.stuff.

How Has This Been Tested?

Tested this locally on my mac. Should work on most *nix systems, but I'm not certain how it will impact Windows.
Made sure to have files that actually import with the top-namespace.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

@ryaminal
Copy link
Contributor Author

I was really struggling to figure out how to better test this so mistakes like I made in #32 don't happen again.
A simple unit test isn't sufficient, it seems. I wonder how the Poetry project and other Poetry plugins do unit/integration tests? We could do essentially automate what I started to do to test:

  1. create poetry project
  2. create a few files
  3. create another project that will consume project 1
  4. build project 1
  5. install project 1 in to project 2
  6. run a pytest in there

That is so very cumbersome to setup, but not the end of the world. Curious if there is a better way for another PR.

@DavidVujic DavidVujic self-requested a review May 31, 2023 06:12
@DavidVujic
Copy link
Owner

Thank you @ryaminal, I'll have a look!

README.md Outdated Show resolved Hide resolved
@ryaminal ryaminal requested a review from DavidVujic May 31, 2023 23:03
@ryaminal
Copy link
Contributor Author

ryaminal commented Jun 1, 2023

We started using a project published with these changes and it appears to be working. Apologies again for the wasted time with my mistakes.

@DavidVujic
Copy link
Owner

Great, thank you @ryaminal! ⭐ I will have a look this weekend.

Copy link
Owner

@DavidVujic DavidVujic left a comment

Choose a reason for hiding this comment

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

👏 👏 👏

@DavidVujic DavidVujic merged commit 4f6768b into DavidVujic:main Jun 2, 2023
@DavidVujic
Copy link
Owner

v1.30 is out! Thank you for contributing to this project!

@ryaminal ryaminal deleted the allow-paths-for-top-namespace branch June 2, 2023 17:44
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.

None yet

2 participants