Skip to content

Pep 420 native namespace#67

Open
szakitibi wants to merge 16 commits into
collective:masterfrom
szakitibi:pep-420-native-namespace
Open

Pep 420 native namespace#67
szakitibi wants to merge 16 commits into
collective:masterfrom
szakitibi:pep-420-native-namespace

Conversation

@szakitibi
Copy link
Copy Markdown

@szakitibi
Copy link
Copy Markdown
Author

I have applied the PEP 420 native namespace changes, and updated the configurations with plone.meta.

Also bumped the test coverage up to 90%. On that note:

@thet in #43 (comment):

pushing previously uncommited code to not loose this idea. untested yet. comments welcome.

I have not found any use of the parent_site utility, also it seems to ignore childsite directly - see here.
Is there any actual use for the added parent_site utility method somewhere else? Should it be marked deprecated or simply removed?

Additionally I have updated the jobs:

  1. Removed missing test.
  2. Excluded dependencies check, which produces confusing results. It complains about missing requirements such as plone.browserlayer, zope.component and transaction, which are clearly included for a product using Products.CMFPlone.

@gforcada
Copy link
Copy Markdown
Member

The confusing results of dependencies could be because those are not direct dependencies being used? 🤔 z3c.dependencychecker scans the project to decide what is needed and what not, so even if plone.browserlayer is used because it's a Plone site, if there is no import of it, then the dependency is not needed.

It could happen that at some point Plone drops plone.browserlayer and then you are still pulling it although it is not needed directly by your code.

One question outside this nitpick: I see you are keeping constraints.txt and requirements.txt but I don't see them used anywhere. Is that on purpose? Or I fail to see where they are used?

@szakitibi
Copy link
Copy Markdown
Author

@gforcada

The confusing results of dependencies could be because those are not direct dependencies being used? 🤔 z3c.dependencychecker scans the project to decide what is needed and what not, so even if plone.browserlayer is used because it's a Plone site, if there is no import of it, then the dependency is not needed.

It is the other way around. E.g. plone.browserlayer is not listed in setup.py, but plone.browserlayer.layer has an import in adapters.py. Thus a direct dependency. On the other hand Products.CMFPlone is added to the install_requires list, which has a bunch of dependencies pulled in, including plone.browserlayer.

Adding missing dependencies directly makes dependencies check happy, but there are other problems then - see Meta results here. Still not sure why.

Also it feels strange to list Acqusition, zope.component, plone.browserlayer or transaction, when Products.CMFCore and Products.CMFPlone is already there. I still see them as redundant entries, but that is an opinion.


One question outside this nitpick: I see you are keeping constraints.txt and requirements.txt but I don't see them used anywhere. Is that on purpose? Or I fail to see where they are used?

I'm new to the plone.meta concept, so I simply used it and took collective/collective.easyform#470 as a reference for the changes. I did not check if it is in use or not. In EasyForm requirements.txt got updated and not removed - see commit 0a305757 or the file. Probably it was a change as preparation for this commit.

@szakitibi
Copy link
Copy Markdown
Author

@gforcada
I have cleaned up things. Restored dependencies check, removed requirments.txt and constraints.txt. The solution for passing dependencies check is to drop Products.CMFPlone in requires list, which is odd for a Plone add-on.

The question about parent_site is still to be answered - see #67 (comment).

@szakitibi
Copy link
Copy Markdown
Author

All checks green. It is currently not visible on PR. The action run can be seen on the fork: https://github.com/szakitibi/collective.lineage/actions

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.

2 participants