Skip to content

[AssetMapper] Detect import with a sequence parser #59004

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 25, 2025
Merged

[AssetMapper] Detect import with a sequence parser #59004

merged 1 commit into from
Jan 25, 2025

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Nov 27, 2024 •
edited by OskarStark
Loading

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #58928 and #58944 (maybe)
License MIT

Third attempt after #54134 and #58999.. this is the good one 👍

I initially started this PR to learn / try things, but it really behaves well.. and may stop (or help stopping) what seems to be a never-ending suite of special cases.

Open to any feedback

@carsonbot carsonbot added this to the 7.3 milestone Nov 27, 2024
@carsonbot

This comment has been minimized.

Sign in to view
@smnandre
Copy link
Member Author

Dear Carson.... you're doing a bad ChatGpt impression right now 😅

@smnandre smnandre mentioned this pull request Dec 23, 2024
Closed
@smnandre smnandre mentioned this pull request Jan 19, 2025
Closed
dunglas
dunglas reviewed Jan 19, 2025
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for working on this. This looks good to me overall, but my review was very very quick and I didn't compared with mainstream JS parsers so I may have missed things.

@smnandre
Copy link
Member Author

Thank you for working on this. This looks good to me overall, but my review was very very quick and I didn't compared with mainstream JS parsers so I may have missed things.

No worry.. and thank you for taking the time :)

My main question is currently: how internal can/should we make this ? Maintaining for AssetMapper is already something, but probably not something we want to publish with BC promise, documentation, etc?

(will address your feedback later tonight!)

@dunglas
Copy link
Member

dunglas commented Jan 19, 2025

I would make it entirely internal

@smnandre smnandre force-pushed the feat/js-sequence-parser branch from a17a895 to b061eca Compare January 19, 2025 20:24
@smnandre
Copy link
Member Author

@dunglas @OskarStark if we use it only in this specific use case, we don't need the value object, because we don't need to keep the previously matched sequences.

That's why i simplified the code here to focus only on the current sequence.

@smnandre smnandre requested a review from dunglas January 24, 2025 23:11
@fabpot fabpot force-pushed the feat/js-sequence-parser branch from 1df8c8b to 720c387 Compare January 25, 2025 08:25
@fabpot
Copy link
Member

fabpot commented Jan 25, 2025

Thank you @smnandre.

@fabpot fabpot merged commit 4fdf3f7 into symfony:7.3 Jan 25, 2025
2 of 5 checks passed
@marsaldev
Copy link

@smnandre thank you!
do you think you could fix this also on the 6.4?

@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot approved these changes

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
AssetMapper Bug Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

api platform no longer installs seamlessly with Symfony
6 participants
@smnandre @carsonbot @dunglas @fabpot @marsaldev @OskarStark