Improve algorithm for imports sorting and remove an autoload #33
Conversation
Such keyword isn't a thing in Purescript, it has instead syntax `import Foo as Bar`. I also checked VSCode purescript highlight mode, it also doesn't have `qualified` in among its sources.
|
@purcell hello, new PR, and there are two others in the having been hanging for a while as well. I'd just kindly remind that I am open to taking a co-maintainership over the project 😊 |
|
Btw, while at it: I've been thinking to remove from the CI matrix the minor Emacs versions, what do you think? It's just that I don't see the point of testing e.g. 25.1, 25.2, 25.3. They have all the same functional and are only different in fixes. I'd imagine it may be more useful to either only test 25.1 (so we cover the "buggiest" Emacs version) or 25.3 (so we assume the user has updated to the latest minor version). |
This improves debuggability of fails, because tests would only show the part that failed. There's no reason to wrap full code blocks to `should` because their execution may only exit past `should` if exception is thrown, which would be caught by `ert` anyway.
The function will be brought into the scope by purescript-mode by virtue of having a `(require 'purescript-sort-imports)`. No point additionally declaring it as autoload.
Hi-Angel
force-pushed
the
fix-sorting-import
branch
from
05b42fb to
4c2d188
Compare
What's the change visible to users here? |
None. It just removes clutter from the old code. The project was based on |
|
Got it, makes sense, I'd forgotten that there's no "qualified" in Purescript 👍 |
|
Thank you! What do you think about this, btw, would you accept such PR? |
Btw, I don't want to look like a bad contributor, so I'd just point out these details are mentioned in the commit description 😅 I put to the PR description just a short summary of the changes done by the commits |
|
Thanks for writing good commit messages, this is widely neglected. 💪 My 2c of advice: if you're going to write a PR description (yay!), that's the main communication to the maintainer, and inspecting each commit should be optional: the primary thing to capture when making a contribution is "why?", because it translates to "why should the maintainer care about merging this?". Here the PR title strongly implied you'd changed the sorting algorithm, which sounded like a functionality change, hence my question. |
|
BTW, sent you an invite to join this org. I'd still recommend you open pull requests for review & visibility rather than pushing changes directly, but this should at least streamline the contribution process for you. Thanks for helping out! |
Thank you!
Thank you, sure! I will be leaving pull requests open for some days for review and more testing by me, and then if nobody comments on them would merge. |
No point in testing minor Emacs releases, because they have no major features anyway, fixes only. E.g. given Emacs 25: 25.1, 25.2 and 25.3 are only different in amount of fixes. It isn't expected that something may break between these releases. So there's not much point in running tests on all of them. This commit removes most minor Emacs versions and only leaves in the initial release for each Emacs major version. It was brought up here before purescript-emacs#33 (comment) so far to no opposition.
This PR basically does two things: stops considering
qualifiedas part of PureScript syntax, and removes extraneous autoload from the sorting function (the sorting is only useful as part of purescript-mode, which should bring the function into the scope by virtue ofrequireing the module).It may be desired to remove
qualifiedfrom elsewhere as well, but there're a lot of things to improve, so let's do them one small step at a time 😊