graphite-project / graphite-web

A highly scalable real-time graphing system
http://graphite.readthedocs.org/
Apache License 2.0
5.89k stars 1.26k forks source link

Fix pyparsing > 3.0 compatibility issue. #2727

Closed parrotpock closed 2 years ago

parrotpock commented 2 years ago

Addresses issue raised in https://github.com/graphite-project/graphite-web/issues/2726 plus one other compatibility issue I came across with a failing piped query test.

I've checked that tests pass but haven't tested this in a live environment. Is it worth adding something to the tox matrix to check multiple pyparsing versions? It's been a while since I've used tox, so I left this part out as I'm not setup to test it properly.

deniszh commented 2 years ago

Hi @parrotpock ,

Thanks for your contribution!

Is it worth adding something to the tox matrix to check multiple pyparsing versions?

As far as I understand tox it's already checking multiple pyprsing versions implicitly. But maybe I'm wrong.

Please also take note, that currently pyparsing version is pinned, so, your change doesn't do much: https://github.com/graphite-project/graphite-web/blob/master/tox.ini#L39 https://github.com/graphite-project/graphite-web/blob/master/requirements.txt#L46

You need to relax constraits to proper check your changes.

parrotpock commented 2 years ago

Yeah, I only tested this manually with the updated dependencies. Should I drop the pin in both places or just the tox config for now?

codecov-commenter commented 2 years ago

Codecov Report

Merging #2727 (fb440b1) into master (e33a273) will decrease coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2727      +/-   ##
==========================================
- Coverage   77.11%   77.09%   -0.03%     
==========================================
  Files         176      176              
  Lines       18972    18972              
  Branches     4034     4034              
==========================================
- Hits        14630    14626       -4     
- Misses       3812     3814       +2     
- Partials      530      532       +2     
Impacted Files Coverage Δ
webapp/graphite/render/evaluator.py 90.19% <100.00%> (ø)
webapp/graphite/render/grammar_unsafe.py 82.25% <100.00%> (ø)
graphite/tags/redis.py 87.03% <0.00%> (-1.24%) :arrow_down:
webapp/graphite/tags/redis.py 87.03% <0.00%> (-1.24%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e33a273...fb440b1. Read the comment docs.

deniszh commented 2 years ago

Yes, please. Minimal pin has sense, but drop < 3.0.0 part

deniszh commented 2 years ago

Tests are green, but looks like I was wrong - now it's testing only latest pyparsing, which could be OK, but including also pyparsing2 tests could be good idea too.

ptmcg commented 2 years ago

As the maintainer of pyparsing, I suggest that at minimum you test against pyparsing 2.4.7 and 3.0.6. 2.4.7 has been the stable release since April, 2020, and 3.0.6 is the most recent released version including all regression and compatibility bug-fixes encountered in 3.0.x.

parrotpock commented 2 years ago

I added a new target for the suggested pyparsing version and above. Since we removed the upper pin this seems vaguely sensible? I could probably rename the target "pyparsing3andabove", but went instead for brevity :)

Thanks for the quick feedback, btw.

deniszh commented 2 years ago

Tests are green. Thanks a lot, @ptmcg , for suggestions! And @parrotpock ofc for fixing that.

parrotpock commented 2 years ago

No problem @deniszh. Let me know if you need anything else from me before you can merge this.

deniszh commented 2 years ago

I think it's good enough to merge. Thanks!

parrotpock commented 2 years ago

Thanks @deniszh!

deniszh commented 2 years ago

💔 Some backports could not be created

Status Branch Result
1.1.x Aborted

To backport manually run: node scripts/backport --pr 2727. For more info read the Backport documentation