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

Ignore whitespace changes to functions #50

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

Conversation

emilbayes
Copy link
Contributor

Hi! Thanks for the great library and the migra tool.

I'm not sure this change is entirely fool proof, but I can't think of a case where whitespace has special meaning, unless the embedded language used is whitespace sensitive (such as Python itself).

At least it has been useful to me when formatting the body of functions for readability, but not making any functional changes beyond whitespace

Split function definitions by whitespace, and compare the lists of tokens for equality
@emilbayes
Copy link
Contributor Author

I think this might be similar to #37. The approach here is probably too naive. A full tokenizer is probably required to be able to do this proper

@djrobstep
Copy link
Owner

Yeah, the trouble with this is that if you have whitespace literals within your functions, this won't work correctly.

I understand why this feature would be useful (I've wanted it myself many times), but I'm not aware of a way to do this without compromising on correctness.

I think a better approach might be highlighting what has changed about a function via the addition of comments in the output. You could even show a text diff of the definition.

I'm working on some structural changes to the codebase that will allow the addition of such comments easily, but it will take some time.

@emilbayes
Copy link
Contributor Author

Yeah I have a hacked in place ndiff also to help me out. I activate it with an env var so I didn’t have to make too many changes throughout

@djrobstep djrobstep force-pushed the master branch 3 times, most recently from 13f62b5 to ebbc2e4 Compare September 18, 2022 05:57
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