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

New approach for lime package management #281

Merged
merged 15 commits into from Jun 4, 2014
Merged

New approach for lime package management #281

merged 15 commits into from Jun 4, 2014

Conversation

zoli
Copy link
Member

@zoli zoli commented May 14, 2014

This is a suggestion for lime package managing.
For now we have these package types:

  • Settings
  • Keybindings
  • Syntaxes
  • Plugins
  • Themes

Each of them will be a type implementing Package interface, The init function will be responsible for scanning paths, finding packages and keeping the hierarchical priority
For each Package type the Get() method will be responsible for returning useful data that we will need for adding that package for example for a Plugin it might be returning python files or for a Setting returning the the data in the settings file, The important point is we won't load the package we just will prepare it for loading the package it would be something like this:
Settings

for _, setting range packages["settings"] {
        loaders.LoadJSON(setting.Get(), e.Settings())
}

Or Plugins

for _, plugin range packages["plugins"] {
        for _, f range plugin.Get(){
                 s, err := py.NewUnicode(plugin.Path() + "." + f.Name()[:len(f.Name())-3])
                 m.Base().CallMethodObjArgs("reload_plugin", s)
        }
}

If this approach is fine i will continue committing to this branch until it's done.

)

type (
Package interface {
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me what the intent behind the interface and its methods are, please add a brief comment in the code on what each function is meant to accomplish

@zoli
Copy link
Member Author

zoli commented May 14, 2014

It's more clear now with using the new approach for loading plugins.
BTW still there is work to do it's not clean and some more functionality for reloading packages will be needed, I just wanted to show what I'm tending to do.

// a keymap will be the file data
Get() interface{}
// Returns the path that the package exists
Path() string
Copy link
Member

Choose a reason for hiding this comment

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

Nothing references this method. Is it really needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure really i thought it might be needed on reloading or something else.

@quarnster
Copy link
Member

Btw, I don't mean to be difficult, just for the backend package I'd like to keep it as slim as possible and as well designed as possible, especially when introducing new interfaces. Ideally only methods and API's that must be exported (i.e. used by a frontend or to enable plugin-flexibility) and in this package should be, the rest should be unexported or even better in another package. (Opened up #282 to deal with cleaning up existing code there.)

I'm asking the questions because I don't know the answers myself, and I'm hoping that you or someone else watching this project might get some ideas from them. So please see my questions as trying to guide the thinking around the api and what it should accomplish rather than as a stick to punish you with :)

@quarnster
Copy link
Member

Also, if you'd like more time to work on this, feel free to close the pull request and open up a new one when you are ready for a review.

@quarnster
Copy link
Member

(btw2: please raise questions like this for existing code where it doesn't make sense as you stumble upon it, I'm sure there are many "wtf?!" code bits in there)

@quarnster
Copy link
Member

Another point to consider is that ST3 can store packages in a zip file named "*.sublime-package", so the .py, .sublime-settings etc would all be contained inside of that zip file.

@zoli
Copy link
Member Author

zoli commented May 15, 2014

When i started reading limetext code about 2 months ago i was thrilled by the design of backend truly it's elegant and clean so i was worried that i might damage the design, I maid pull request before completing the feature for getting some feedback to be sure i'm in right path.
Being somehow difficult is really necessary for keeping the design slim, I will keep this request open for pushing more commits to resolve the concerns, implementing the reload functionality and dealing with zip packages.
Btw, I have more free time these days and will try to make more contribution to move this project forward.

@zoli
Copy link
Member Author

zoli commented May 27, 2014

Except reading Packages from zip files, I think we're done here. Reloading and scanning plugins, settings, commands and etc are enabled even the backend has not much problem with loading plugins with different languages than python we can scan plugins with special suffix.
Any idea why travis is failing all tests are ok in my system?

@quarnster
Copy link
Member

sublime_test.go:27: 00002960 162 [2014/05/27 10:34:37 UTC] WARN open ../../3rdparty/bundles//VintageousCHANGES.txt: no such file or directory

Looks like a '/' is misplaced? Are you running the tests via ./tasks/ci/lime.sh in $GOPATH/src/github.com/limetext/lime?

@zoli
Copy link
Member Author

zoli commented May 27, 2014

I was using go test ./backend/... but with ./tasks/ci/lime.sh works fine to I am receiving SUCCEEDED for both backend and frontend/termbox


// Initializes a new plugin whith loading all of the
// settings, keymaps and python files inside the path
func NewPlugin(path string, suffix string) *Plugin {
Copy link
Member

Choose a reason for hiding this comment

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

Who makes use of this function? What does the "suffix" parameter do? Please add comment in code

@quarnster
Copy link
Member

Again, I hope my questions and comments aren't too demotivating. Many them are rhetorical in nature and not to imply that what you've written isn't good. My hope is to awake discussion, insight and a well defined api with clear documentation (and again, same goes for existing code already written, please question it where it doesn't make sense and add documentation where needed).

But let me be clear: I do not require you to make all changes suggested here before merging. We can merge now, open up a separate issue number to deal with remaining items and you can work on something else if this is becoming tedious :)

IMO it's better to move forward and make incremental improvements than to have everything be perfect on first try. Some of these things we've discussed here isn't 100% important for a 0.1 release for example, but they are good to start discuss so that we have a better idea of the problems involved and the possible solutions once we reach 1.0.

@zoli
Copy link
Member Author

zoli commented May 29, 2014

Not at all,I really like to help this project move forward and my motivations are the same as mentioned in limetext readme

People including myself were wondering if the product was dead and I personally wondered what would happen to all the bugs, crashes and annoyances that still existed in ST2.

Also I am learning here that how should i think on designing a program and what questions should i ask myself before creating different components.
Sorry if i am taking your time too much.

I will fix some parts that might cause errors for merging and will create separate issues for other problems.

@quarnster
Copy link
Member

No need to apologize, your help is very much appreciated and like I said, we don't need to be perfect on first try and certainly not for 0.1.

This stuff is good to discuss and think about so that once we reach 1.0 we are happy with how all parts fit together. By exchanging ideas and discussing different needs we'll get a better plan for all the different parts, even if we don't end up solving all of them or will need to throw out code and re-write if we find out that the current design is insufficient or has problems that we didn't think of back then.

Same goes for all existing code. If it doesn't make sense, is too complex, could be moved to another package, etc it should be discussed so that we can either figure out why it looks like it does (and add in comments about it in the code and/or wiki) or come up with an improved solution.

In that spirit, please don't see this as taking up my time or any one else's. Discussing the details and trying to figure things out are very much part of the process and are absolutely necessary in arriving at a satisfying solution. Personally I'd be more worried if there wasn't any discussion as that would mean we aren't thinking enough about the code or that we are too similar and aren't able to think of potential problems that perhaps others could :)

@quarnster
Copy link
Member

Thanks for opening up separate issue numbers for the various things we've discussed here. Are you still working on any bits here or looking for feedback? If not, lets merge and we can deal with the remaining bits in these other issue numbers.

@zoli
Copy link
Member Author

zoli commented Jun 3, 2014

I'm working on some easy fix problems that will be finished by tomorrow after that we can merge this.

… variable

and pckts type unexported, we don't keep packet data anymore reloading plugin is better now check #281 for more details
@zoli
Copy link
Member Author

zoli commented Jun 3, 2014

I think we're done here. This could be merged

quarnster added a commit that referenced this pull request Jun 4, 2014
New approach for lime package management
@quarnster quarnster merged commit dc20246 into limetext:master Jun 4, 2014
@quarnster
Copy link
Member

Cheers, let's continue discussion in one of the newly filed issues or open up a new one if one is missing.

@zoli zoli deleted the feature/lime-packages branch June 4, 2014 12:12
@quarnster quarnster mentioned this pull request Aug 25, 2014
@zoli zoli mentioned this pull request Jun 17, 2015
25 tasks
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.

None yet

2 participants