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

Further cleanup #223

Merged
merged 30 commits into from
Nov 23, 2019
Merged

Further cleanup #223

merged 30 commits into from
Nov 23, 2019

Conversation

Gastove
Copy link
Contributor

@Gastove Gastove commented Nov 1, 2019

Note: This PR is the fourth in a series starting with #219. It continues work detailed in #218.

What's here?

This PR is the last of my "notes and documentation" PRs. It adds more tests and more code-comments to help orient further work. However: I've also hit the point where testing is beginning to turn up bugs. To keep bugfix and improvement PRs short and to the point, this PR includes some disabled tests. The next PR will be the first PR that starts deeply changing the meaning of the code, rather than simply organization and naming.

Clean up tabs, indentation.
These are two modules that do tightly related things. In two separate files,
they can't cleanly share variables (`indent-smie` re-defined an indent offset
var also defined in `indent`). This commit merges them together and re-names the
variable used for offset by the `smie` configs, but does nothing else.
This commit renames `fsharp-mode-indent` and cleans up references to it. It also
scoots `fsharp-eval-phrase` next to `fsharp-eval-region` in `fsharp-mode`, where
it makes a bit more sense.

Tests are all currently passing.
This commit removes some old "major mode boilerplate" from
`fsharp-mode-structure`:

- The abbrev table is moved in to `fsharp-mode`, where it belongs.

- `fsharp-safe` is _only_ used to wrap calls to `search-backwards`; the only
value it provides is to swallow errors and instead return `nil`. However: this
can now be much more idiomatically achieved by passing `t` as the `NOERROR`
argument to `search-backward`.

- XEmacs hasn't had a release in ten years. The extra region setting calls of
`fsharp-keep-region-active` aren't needed -- it's just extra noise.
This allows it to do precisely the same thing, while making very sure to fulfill
the guarantee of not changing whatever point and mark the user set.
`fsharp-compute-indentation` is one of the critical driving methods of this
mode's indentation system. At the start of this commit, it was also a very long,
very complex method. It needs better documentation and it needs to be under
test. Therefore: extract method.

This commit pulls a variety of method out of `fsharp-compute-indentation`,
seeking to preserve basically identical functionality while capturing pieces of
logic in smaller, better documented pieces. Functions are currently named after
my best read of what they _do_, but that's hard to be confident about.

The _next_ piece of work will be getting these functions under test, which will
allow me to pin down and describe their functionality (and its limits!).
This puts `fsharp-nesting-level` under test, and updates the docstring of that
function with information secured during testing.
This is all notes, comments, docstrings, spacing. Preparation for further work.
Group related functions; move some `info-look` configs into `fsharp-mode.el`.
This is getting to be a lot easier to navigate, look at, understand all
together.
Originally, all of these functions were inside one huge save-excursion form. To
my surprise, they typically need their _own_ save excursion wrapping to make
sure everything is consistent, particularly under test.

Also added, a great deal of notes about the function itself and how it works.
`fsharp-compute-indentation` is a huge cond, and uses a final `t` case as its
default condition. This moves that condition into its own function (complete
with save-excursion) so that it can be tested on its own terms.
I've built a whole bunch of new testing aaaaaand it has uncovered things that
don't work! This PR is already big enough, so I'm adding these tests commented
out, and will start bugfixes in the next PR.
@juergenhoetzel
Copy link
Collaborator

Thanks for many thanks for all the work and sorry for the delay (lot of microcommits)

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