Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Package rewrite #309

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Package rewrite #309

wants to merge 17 commits into from

Conversation

fluvf
Copy link

@fluvf fluvf commented Sep 14, 2020

Description of the Change

This pull request aims to

  • Implement LSP snippets, while still allowing for legacy textmate syntax
  • Improve code base health
  • Improve provided service API
  • Better support multiple cursors
  • Support snippets inside snippets
  • Probably more as I clean this up

This pull request is still partly incomplete. I've decided to open this now to see if the changes are wanted and to essentially get a second opinion
Unfortunately breaking this pull request into several smaller ones would be too much effort, and
temp code that'd just be removed later, for it to be worth it.

Currently the commits are still large and unorganized, thus (depending on the response) I will still:

  • Thoroughly document the code (this will include some thorough manual testing)
  • Update tests
  • Rewrite the git history to be a little more... descriptive 😄

Alternate Designs

No alternate design for the implementation as a whole were considered, but some were for how choices should be displayed.
Designs considered:

  1. Iterating using a keybind in a row
  • Wasn't chosen as long choices could cause them to be offscreen
  1. Iterating using a keybind replacing text
  • Has the drawback of not showing all possible choices
  1. Choosing an option from a dropdown list
  • Harder to implement and maintain

Currently 2 is implemented.
I'm against implementing 3 using a custom dropdown list element, as it'd lessen maintainability, but I'm still exploring other possible methods

Benefits

As with the changes above, I'll fill this as I finalize this pull request

Possible Drawbacks

This isn't backwards compatible
Most of the internal structures / logic are rewritten, as is the service API
To combat this breaking everything, I will be opening pull requests for bundled packages to update their usages of snippets

Applicable Issues

Most of them? Again, this is a section that I will update while finalizing this pull request

Outstanding questions

(Yet another thing I'll expand as I go)
Some questions I'd like to get feedback on:

  • Is scoped-property-store still an acceptable way to store the snippets?
  • Is season still and acceptable CSON parser?
  • Is the ´onDidLoadSnippets´ helper still needed?
  • Are the warning notifications on snippet load failure too invasive?

fluvf added 12 commits July 19, 2020 01:14
The project mostly already followed standard style
This'll help to catch any errors in new code and keep their style consistent
And while we're at it remove now unused `coffeelint`
Fixes a few vulnerabilities reported by npm

The reorder isn't necessary, I just thinks it looks better
Most of the changes we're made by `standard` automatically
It's only used by the specs
Master has had a great number of improvements and bug fixes after the latest stable `0.10.0` version, which is over four years old by now.
Simplifies the code and file structure significantly.

Unfortunatelly we can't spy that the getter is invoked in specs, as the jasmin version included in atom is ancient.
we'll see if ive made a huge mistake
needs:
- tests
- documentations (will result in some extensive manual testing)
- choices (not sure how the dropdownlist will be implemented)
@fluvf fluvf changed the title project: Rewrite Package rewrite Sep 14, 2020
choices are now iterated over using shift-tab
downside is that available choices aren't visible
alternative could be inserting all chioces, and setting
cursor selection to the currently choces one
the downside being extremely long choices that
could expand past the screen
@savetheclocktower
Copy link
Contributor

I'm not the decision-maker on this, but if you want feedback from a contributor:

This is ambitious work — and good work, from what I can tell — but I think you're better off coordinating with @Aerijo's new ground-up snippets package.

I'm really torn here. I think it's risky to accept such big changes all at once, because it takes a codebase that its contributors are all broadly familiar with and turns it into one that only the PR author really understands.

In theory, if a PR like this could demonstrate new functionality without any regressions in the test suite (except for breaking changes, which would need to be kept to an absolute minimum), I'd be in favor of it, but only if I were convinced that there was no better way to get there incrementally.

@Aerijo
Copy link
Contributor

Aerijo commented Sep 15, 2020

I was advised to split it into independent PRs instead of trying for a monolith of changes.

@Aerijo
Copy link
Contributor

Aerijo commented Sep 18, 2020

The reason I made a separate package is to get the freedom of making massive changes, without the risk of breaking anything bundled with Atom. The history starts here. If I use it enough and iron out the bugs, I might revisit and start making PRs again. To my understanding this is why the builtin autocomplete package is called autocomplete-plus; the plus version was originally a community package to replace the builtin autocomplete. I don't think you'll find anyone to authoritatively merge such a large PR as is though. Splitting it into smaller steps would be necessary.

Backwards incompatibility would make this dead in the water. You could make PRs to Atom organisation packages sure, but fixing all community packages would be a nightmare. Breaking changes have been done before, but that was when the team was larger and everything seemed more active in general.

So you would need to make the interface changes opt-in; typically by making a new version for services, and a config setting for the snippet syntax to be used. It looks like you have a legacy syntax flag though, so I'm not sure what else would be incompatible?

Is scoped-property-store still an acceptable way to store the snippets?

Do you have an alternative? It's not perfect, but it's generally good enough.

Is season still and acceptable CSON parser?

I believe it's the only one used in the Atom repos. We only use the basics anyway for snippets, and it's not like they have had breaking changes.

Is the ´onDidLoadSnippets´ helper still needed?

IIRC It's used by the settings-view package. Think when the settings are open but the snippets haven't been loaded yet.

Are the warning notifications on snippet load failure too invasive?

Typically would add a "don't remind me again option" to blanket disable them.

@fluvf
Copy link
Author

fluvf commented Sep 18, 2020

I've started to break this into bite sized chunks, first one being #310
Once I'm done, the big changes shouldn't be larger than 1000 lines

What I mean by this being backwards incompatible are mainly the snippetsForScope and getUnparsedSnippets functions offered by master
The new snippetsByScope doesn't offer the exact the same functionality, as it just exposes the underlying data-structure,
and there is no equivalent for getUnparsedSnippets
If they're actually used, I can add functions with deprecation warnings for them

IIRC It's used by the settings-view package. Think when the settings are open but the snippets haven't been loaded yet.

I'm pretty sure that the settings-view package is broken when it comes to snippets, as I can't see any difference regardless if a package has defined some or not
After a quick look at the code I got the impression it's calling a non-existent function

If nothing else, and if @Aerijo you're willing, I could port these changes to your fork
But in the meantime, I'll finish these pull requests for this repo

im second guessing myself
i made some false assumptions about how stuff worked
But now this should be onpar with master, kinda
The babel stuff will be removed once i clean this up
i made some refactoring and such
this is more of a "hey, this is till being worked on" commit
stuff still to do:
- transformations are not done, i changed the format (easy)
- tests are incomplete (being worked on, not on this commit) (easy but time consuming)
- need to go over things and document them properly (hard, but i think i might have something to make it easier)
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.

None yet

3 participants