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

Declare dependency on lxml #1001

Closed
wants to merge 1 commit into from
Closed

Conversation

rowillia
Copy link
Contributor

Mypy depends on lxml for reports but doesn't declare it as a dependency
so it doesn't get installed alongside of mypy. Also, mypy no longer
works with latest release of lxml so I had to pin to the previous release.

Mypy depends on lxml for reports but doesn't declare it as a dependency
so it doesn't get installed alongside of mypy.  Also, mypy no longer
works with latest release of lxml so I had to pin to the previous release.
@JukkaL
Copy link
Collaborator

JukkaL commented Nov 29, 2015

Thanks for the PR! Have you tested that this works in Windows? I've heard people complain about installing lxml in Windows.

In the longer term I'd rather get rid of the lxml dependency (see #909).

@rowillia
Copy link
Contributor Author

@JukkaL I don't have a windows box :(.

If we don't want to support depending on lxml, should we disable the reports that need it if it isn't installed?

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 1, 2015

Yeah, detecting if lxml can be imported and falling back to more basic behavior would be totally fine.

I have a Windows VM and I can try this later when I have time, but I have a bunch other stuff I need to finish first.

@gvanrossum
Copy link
Member

@rowillia in #1120 I'm fixing the problem reported in #1002. Can you review that? (I'm not so clear on XML, but I got it to install and work. :-) Once that's in, you can redo this one without the pinning.

@gvanrossum
Copy link
Member

This is still awaiting a test for Windows. Honestly the report generation code is so flaky that I don't think we should merge this at all. However, I fixed the lxml issue, so that it now works with lxml 3.5, so at least the pinning is no longer necessary.

(FWIW I also added some more pep8 exceptions so the flake8 pinning is no longer necessary either. And somehow the Travis CI setup wasn't using test-requirements.txt anyways.)

@gvanrossum
Copy link
Member

I'm closing this -- we should do this differently anyway, by removing our lxml use.

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.

3 participants