Skip to content

Conversation

shufo
Copy link
Contributor

@shufo shufo commented Jan 14, 2025

Context

Problem

Solution

Skip comments and whitespace when peeking as peek() is just peeking next token without consider comment. I don't know this is proper solution but it works as expected in my environment.

@shufo
Copy link
Contributor Author

shufo commented Jan 15, 2025

Tests has passed but seems benchmark failed. I don't know it's ok to failed.

Run python cachegrind.py node test/benchmark2.js > output.txt ==2711== Cachegrind, a high-precision tracing profiler ==2711== Copyright (C) 2002-2017, and GNU GPL'd, by Nicholas Nethercote et al. ==2711== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info ==2711== Command: node test/benchmark2.js ==2711== ==2711== ==2711== I refs: 237,39[5](https://github.com/glayzzle/php-parser/actions/runs/12772711343/job/35611721161?pr=1152#step:7:6),387 Traceback (most recent call last): File "/home/runner/work/php-parser/php-parser/cachegrind.py", line 137, in <module> print(github_action_benchmark_json(combined_instruction_estimate(get_counts(run_with_cachegrind(sys.argv[1:]))))) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/runner/work/php-parser/php-parser/cachegrind.py", line [10](https://github.com/glayzzle/php-parser/actions/runs/12772711343/job/35611721161?pr=1152#step:7:11)0, in get_counts ram_hits = d["DLmr"] + d["DLmw"] + d["ILmr"] ~^^^^^^^^ KeyError: 'DLmr' 
claytonrcarter added a commit to claytonrcarter/plugin-php that referenced this pull request Apr 18, 2025
This is a feature regression, but it's needed to workaround a defect in php-parser introduced in v3.2.2. I feel like this is an acceptable regression that we can un-regress once the pending fix is merged and released in php-parser. Ref: glayzzle/php-parser#1152
@czosel
Copy link
Collaborator

czosel commented Jun 25, 2025

@shufo sorry about the late response! We're looking to fix the CI in #1157.

@czosel czosel force-pushed the fix/multiline_class_constant_with_comment branch from b7ff101 to 0965288 Compare June 25, 2025 15:05
Copy link
Collaborator

@czosel czosel left a comment

Choose a reason for hiding this comment

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

The code changes looks good to me! 👍
I rebased on top of #1157 - with those changes, CI seems to be green as well. We'll get this merged as soon as #1157 has landed.

@czosel czosel force-pushed the fix/multiline_class_constant_with_comment branch from 0965288 to 5d09ee6 Compare June 27, 2025 22:06
@czosel czosel merged commit a296eab into glayzzle:main Jun 27, 2025
3 checks passed
@czosel
Copy link
Collaborator

czosel commented Jun 27, 2025

Released in v3.2.4 🎉

cseufert pushed a commit to claytonrcarter/plugin-php that referenced this pull request Jul 9, 2025
This is a feature regression, but it's needed to workaround a defect in php-parser introduced in v3.2.2. I feel like this is an acceptable regression that we can un-regress once the pending fix is merged and released in php-parser. Ref: glayzzle/php-parser#1152
@shufo
Copy link
Contributor Author

shufo commented Jul 11, 2025

@czosel
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants