Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

file parsing solution #9

Merged
merged 16 commits into from
Feb 26, 2016
Merged

Conversation

kbrose
Copy link
Contributor

@kbrose kbrose commented Feb 23, 2016

Overview

This PR is a work in progress.

This version of the code does a rudimentary parsing of the python source code, keeping track of brackets and doing _very_ rudimentary bracket matching (if the python source code is not well-formed then bad things could happen).

Implementation notes

(A note on terminology, I use "bracket" to refer to any of the [](){} characters.)

Essentially, simple parsing is performed to keep a stack of the column-location of open brackets (one of [({). When an opening bracket is read, it adds to the stack indicating the column where the bracket is located, when a closing bracket (one of })]) is read, it pops the latest element (it does not do any checking to make sure the closing bracket matches the opening bracket - this is why it assumes the python source code is well-formed). If there is anything in the stack after parsing the python file up to the cursor location, then that means there is an open bracket. We can see what column the most recent addition to the stack was on, and then set the indent from that.

TODO

Ordered in what I perceive to be most important (top) to least important (bottom):

  • Should intelligently un-indent back to the correct level after all brackets have been closed.
  • All of the settings and things are still in the file. They will need to be updated (to only include the amount of the hanging indent?)
  • Only supports soft-tabs. It seems to me that the pep8 convention does not even make sense with hard-tabs, since it explicitly requires being able to have preceding whitespace of any amount, not necessarily divisible by the length of the tab. Still, though, it would be nice to support this. Perhaps until we can do it, we should return without any processing if hard-tabs are being used. Looking at https://github.com/atom/atom/blob/master/src/text-editor.coffee#L2702 suggests that you can build an indent string of arbitrary length even when hard-tabs are being used (it just pads with spaces after using as many tabs as possible).
  • I added a python file for testing (test/test_file.py) that has all of the more pathological cases I could think of. Is it possible to automate a test based on this somehow? Don't really know much about testing or testing frameworks.
  • Style-guide and documentation.

Finished

  • This implementation removed the hanging indent feature. Could probably detect the hanging indent the same way as before with the regex. Just need to add it and try it.


# If there's a remainder, `editor.buildIndentString` requires the tab to
# be set past the desired indentation level, thus the ceiling.
# TODO: is the conditional necessary? Maybe to avoid floating point errors?
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this is because of the possibility that rem could be 0, which is totally acceptable. I remember this when testing through everything, though I can't remember the exact scenario.

@DSpeckhals
Copy link
Owner

I made some basic comments on what you have here. Excellent work so far. These kinds of improvements would make the package much better. Thanks!

Edit: I was making some changes/major refactors in the past hour or so. I might merge them this week. They don't really change logic...just module structure/organization. Just keep on working with what you've got, and I can always fix any conflicts that come from your branch later.

@kbrose
Copy link
Contributor Author

kbrose commented Feb 23, 2016

Sounds good.

@kbrose
Copy link
Contributor Author

kbrose commented Feb 24, 2016

Ok, as of db36a0b this is working on all of the test cases (which are all in test.py) I could think of (let me know if I missed any. This was on Windows 10. Whenever you've got the time, could you look it over and make comments on what you want changed? This is my first time writing any JS/coffeescript, so style comments are definitely welcome in addition to practical things.

BTW, I also had a ~600 line python file that I played around with and noticed no performance issues.

lastFunctionRow = parseOutput.lastFunctionRow

if shouldHang
console.log('here')
Copy link
Owner

Choose a reason for hiding this comment

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

As you can probably guess, you'll want to take out the log statements.

@DSpeckhals
Copy link
Owner

I left some more comments. This is getting pretty close! When you're happy with me testing this for a day or 2 in real life, let me know; then I'll pull your branch, resolve conflicts, and try it out.

Added documentation, fixed some style concerns. Only keep track of the
last place a bracket was closed instead of a whole stack since that's all
that's needed for the parsing. Rearranged logic for when hanging indent
should be used to accomodate comments on the line before a hanging indent
is used.
@kbrose
Copy link
Contributor Author

kbrose commented Feb 26, 2016

Added more documentation to the parsing section, fixed the style suggestions, and made a couple other minor changes (one performance based, the other should allow hanging indents to be correctly triggered when a comment was entered before pressing enter for the newline that should have the hanging indent). I believe this version (cd8814a) should be ready for you to test.

@DSpeckhals
Copy link
Owner

Super! I'll hammer away at it tomorrow.

@DSpeckhals DSpeckhals merged commit cd8814a into DSpeckhals:master Feb 26, 2016
@DSpeckhals
Copy link
Owner

I resolved the conflicts without much trouble, and will test over the next 24 hours or so. I should be able to release tomorrow.

The reason I originally put this package together was for purely selfish reasons. I was surprised by the response and help from everyone. Your contributions have made it magnitudes better. I look forward to using it "out in the wild." Thanks so much, again! 👏

@kbrose kbrose changed the title Work In Progress: file parsing solution file parsing solution Feb 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants