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

TODO: _find_and_remove #4

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jknndy
Copy link

@jknndy jknndy commented Apr 30, 2024

Hey @anguswg-ucsb, first time contribution here!

just a few things, are you interested in contributions to this library? if so are you open to adopting a linting standard throughout the codebase? ( personal rec black )

Refactored & resolved # TODO: this is always 0 because we're removing the match

  • Dynamically adjusts offset after modifications for precise match positioning. Resolved # TODO
  • Utilizes single pass through matches.

@anguswg-ucsb
Copy link
Owner

Hello @jknndy

I am happy to take contributions, thanks for making a PR! Ill take a closer look at your PR this afternoon. I was planning on using black but I hadn't done so yet.

Ill take a look at your pull request this afternoon. The tiny bit I looked at the code change, looks like you cleared up a bit of hacky code I had in there. The only thing I need to double check is the make sure the use of -= in place of += is appropriate when calculating the new offset value for the string (i.e.):

# this:
offset += - (end - start)

# versus this:
offset -= - (end - start)

@jknndy
Copy link
Author

jknndy commented May 1, 2024

# this:
offset += - (end - start)

# versus this:
offset -= - (end - start)

The original expression involved adding the length of the replacement string to the offset and then subtracting the length of the replaced substring (end - start) & the updated expression directly subtracts the length of the replaced substring from the offset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants