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.

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

[CLOSED] Replace $& with the whole match #5416

Open
core-ai-bot opened this issue Aug 30, 2021 · 21 comments
Open

[CLOSED] Replace $& with the whole match #5416

core-ai-bot opened this issue Aug 30, 2021 · 21 comments

Comments

@core-ai-bot
Copy link
Member

Issue by MarcelGerber
Sunday Nov 10, 2013 at 19:09 GMT
Originally opened as adobe/brackets#5929

Implements the same special replacement patterns as in the Mozilla Reference.
See this little conversation: adobe/brackets#5840 (comment)

Includes Unit tests.

MarcelGerber included the following code: https://github.com/adobe/brackets/pull/5929/commits
@core-ai-bot
Copy link
Member Author

Comment by njx
Tuesday Dec 03, 2013 at 01:44 GMT

Hi - sorry I haven't gotten to this, been busy with non-Brackets stuff (and the holidays :)) I might not be able to get to it until the next sprint.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Tuesday Jan 07, 2014 at 20:16 GMT

@njx Would you mind reviewing this? Are there any changes to make?

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Jan 10, 2014 at 22:53 GMT

Sorry, my time has been crunched. Let me find someone else who can review it next week.

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Jan 10, 2014 at 22:53 GMT

Unassigning so we can look at reassigning it next week

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Tuesday Jan 21, 2014 at 15:34 GMT

Seems like you forgot to re-assign this@njx. Sorry, but I don't want it to be lost ;)

@core-ai-bot
Copy link
Member Author

Comment by njx
Tuesday Jan 21, 2014 at 17:06 GMT

No, haven't forgotten. We're trying to finish out the sprint (a bit late due to some issues with file watchers) and so we aren't picking up other pull requests right now. Hopefully we'll get back to it in a few days. Sorry!

@core-ai-bot
Copy link
Member Author

Comment by kkarlesk
Friday Jan 24, 2014 at 18:29 GMT

@lkcampbell Would you please take a look at this one?

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Saturday Jan 25, 2014 at 23:28 GMT

@SAPlayer, the parsing code appears to be correct.

I guess one of the things I don't understand is why we have to write our own parsing code for the dollar sign combinations if the Javascript engine already knows how to parse these patterns. Is there a way we could just have Javascript do that work for us?

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Saturday Jan 25, 2014 at 23:56 GMT

I guess I should pose the question for@njx as well since it looks like both of you did a lot of work on the dollar sign fixes. Why are we parsing regexps that contain dollar signs manually if JavaScript already understands how to parse them?

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Sunday Jan 26, 2014 at 17:49 GMT

@njx Answered this question himself in #5840: adobe/brackets#5840 (comment)

@core-ai-bot
Copy link
Member Author

Comment by njx
Monday Jan 27, 2014 at 22:05 GMT

Thanks for digging that up@SAPlayer - I had forgotten the reason :)

@core-ai-bot
Copy link
Member Author

Comment by njx
Monday Jan 27, 2014 at 22:06 GMT

(But there might be some clever way to do it that I hadn't thought of...@lkcampbell if you come up with one, let us know :) )

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Monday Jan 27, 2014 at 22:59 GMT

@njx, I think you guys are correct, we may be losing the context of the match that allows the replacement code to work. Still, I will play around with it a little more today and see if there is another approach.

If nothing else, we should come up with a good comment to document the solution so we don't keep asking this same question every time we look at the code. But until I understand the problem a bit better, I don't know exactly what the comment is going to be.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Tuesday Jan 28, 2014 at 15:57 GMT

Maybe something like this?

FUTURE: Maybe try to use the JavaScript replace() function to do all parsing logic.

@core-ai-bot
Copy link
Member Author

Comment by njx
Tuesday Jan 28, 2014 at 18:19 GMT

If we don't actually have an idea for how to do it, I think the comment should just explain why we think we can't - e.g. "Note that we can't just use the ordinary replace() function here, because the string has been extracted from the original text and so might be missing some context that the regexp matched."

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Tuesday Jan 28, 2014 at 18:24 GMT

Sounds good to me for now. I haven't had any revelations in the last 24 hours. @SAPlayer, can you commit@njx's suggested comment?

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Tuesday Jan 28, 2014 at 19:12 GMT

Just added this comment. Hope everything is fine now.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Wednesday Jan 29, 2014 at 15:56 GMT

@SAPlayer, there is a Travis build fail. Something about pressEnter() not being defined.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Wednesday Jan 29, 2014 at 16:35 GMT

It looks like 5cd2043adb71bf75dc9ec9121a2e5bb45513ccd5 removed the pressEnter() function and that is breaking your code.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Thursday Jan 30, 2014 at 20:34 GMT

@lkcampbell Just fixed the unit tests. Thanks for your hint!

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Thursday Jan 30, 2014 at 22:43 GMT

Looks good. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot