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

pythonPackages.pythonparser: init at 1.3 #54231

Closed
wants to merge 2 commits into from
Closed

Conversation

@clacke
Copy link
Contributor

@clacke clacke commented Jan 18, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@worldofpeace worldofpeace left a comment

Additonally, a more preferred commit msg would be
pythonPackages.pythonparser: init at 1.3

@clacke clacke force-pushed the clacke:pythonparser branch from e149fe3 to 6bdbd9e Jan 24, 2019
@@ -692,6 +692,8 @@ in {

python-utils = callPackage ../development/python-modules/python-utils { };

pythonparser = disabledIf (pythonAtLeast "3.7") (callPackage ../development/python-modules/pythonparser { });

This comment has been minimized.

@dotlambda

dotlambda Jan 26, 2019
Member

Suggested change
pythonparser = disabledIf (pythonAtLeast "3.7") (callPackage ../development/python-modules/pythonparser { });
pythonparser = callPackage ../development/python-modules/pythonparser { };

Where do you get this info from? You should instead add disabled = ... inside the expression.

This comment has been minimized.

@clacke

clacke Jan 29, 2019
Author Contributor

I documented the reason in the commit message. The lexer explicitly refuses to process Python code that is indicated to be Python 3.7, and the tests fail.

I used disabledIf because I saw it used elsewhere in the file, but keeping that information with the rest of the package data makes sense, I just wasn't aware of it. I'll use the disabled parameter instead.

This comment has been minimized.

@clacke

clacke Jan 29, 2019
Author Contributor

This comment has been minimized.

@dotlambda

dotlambda Jan 29, 2019
Member

I documented the reason in the commit message

Please add a comment in the expression as you will have to squash the new commits into the initial one.

This comment has been minimized.

@clacke

clacke Jan 29, 2019
Author Contributor

right, ok

pname = "pythonparser";
version = "1.3";

disabled = pythonAtLeast "3.7";

This comment has been minimized.

@dotlambda

dotlambda Jan 29, 2019
Member

According to m-labs/pythonparser#20, there's not even support for Python 3.6.

This comment has been minimized.

@clacke

clacke Jan 29, 2019
Author Contributor

Good find. Asking for clarification.

This comment has been minimized.

@clacke

clacke Jan 29, 2019
Author Contributor

On the other hand, as it builds and tests on Python 3.6, it still has value there, even though it might only be able to parse 3.5-compatible source. If someone tries to use it on sourcecode it doesn't understand, they will get errors, but if they don't, it can be installed together with some hypothetical 3.6-only package they might need.

@dotlambda dotlambda changed the title python: pythonparser: init at 1.3 pythonPackages.pythonparser: init at 1.3 Jan 29, 2019
@dotlambda
Copy link
Member

@dotlambda dotlambda commented Jan 29, 2019

Please rebase on master now that you're already in maintainer-list.nix.

@clacke clacke force-pushed the clacke:pythonparser branch from ab59a6b to 93c36a0 Jan 29, 2019
@clacke
Copy link
Contributor Author

@clacke clacke commented Jan 29, 2019

Rebased, moved comments from commits to source, squashed, left the change still under discussion as a separate commit.

A couple of tests fail with:

    NotImplementedError: pythonparser.lexer.Lexer cannot lex Python (3, 7)

Disable for Python 3.7 and above.
@clacke clacke force-pushed the clacke:pythonparser branch from 93c36a0 to 16587d4 Jan 31, 2019
@mmahut
Copy link
Member

@mmahut mmahut commented Aug 18, 2019

Are there any updates on this pull request, please?

@FRidh
Copy link
Member

@FRidh FRidh commented Feb 9, 2020

Nothing happening so closing.

@FRidh FRidh closed this Feb 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.