Closed magnetophon closed 6 years ago
Forks are just normal business in open source, get over it.
As I just explained in much more detail in pm to @magnetophon, I consider most of @yassinphilip's changes at best unnecessary. I don't consider static keyword lists for library function fontification an improvement -- you really need to go to much greater lengths there, as some Emacs-based IDEs do, to make this automatic and adjust to the actual contents of the library, provide auto-completion etc.
Worse, rev. fa3aa846c404aed55a464d79d425bde128d5ef3a breaks auto-indentation. To see that, just enter something like this in a new and empty .dsp buffer and hit Enter:
import("stdfaust.lib");
That's why in my fork I went back to @rukano's original version (which I've been using for years in my courses and which has been working flawlessly for my students and myself), and cherry-picked two of the newer commits which I consider actual improvements.
@rukano it's your baby so you decide what's in your repo, but I must say that I don't consider the present HEAD usable. I guess that this carries some weight, as my dept. at JGU university at present is probably the only academic institution using both Faust and Emacs heavily. But I realize that it's different strokes for different folks, that's why I just created my own fork rather than complaining here. In retrospect I should have anticipated the resulting arguments, especially after I committed my "basic" version of faust-mode to the Faust repo upon request. :)
One obvious possibility to cope with this kind of schism and make everyone happy would be to have two branches, something like "basic" and "keyword-enhanced" faust-mode. The former would pretty much be what I have in my fork now, the latter would correspond to what's available here, minus rev. fa3aa846c404aed55a464d79d425bde128d5ef3a. (This really has to go, as it breaks auto-indentation. At least the way it's done here. You can derive from prog-mode, but then you'll also have to implement the hooks for auto-indentation, which wasn't done.)
(NB: If the "keyword-enhanced" faust-mode with the static keyword lists is to stay, IMHO someone has to actively maintain it, i.e., keep up with the ongoing development in https://github.com/grame-cncm/faustlibraries. Otherwise those keyword lists may become outdated pretty quickly.)
I'm also happy to discuss whether there's other stuff in @magnetophon's and @yassinphilip's contributions which might be worth cherry-picking for "basic" faust-mode. Please point me to the corresponding commit and I'll have a look.
Hi @agraef, I'm ok with rehosting the main project somewhere else and I delete this repo and rather fork the other one. I haven't had time to work with it again, and I'm happy with the work of @yassinphilip and @magnetophon , so I guess it should be hosted officially where it's going to be active and mantained.
So, let me know who will host, and how to proceed. Cheers!
@agraef I agree: "Forks are just normal business in open source".
That is not to say it's always the best solution. I for one, would prefer if you and @yassinphilip could find a middle ground that has the advantages of both your forks, and from then on develop this together. Less wasted man-hours, more features for the end-user!
PS: When I filed this issue, I didn't even think of the possibility that agraef's fork was intentional! ;)
I wouldn't mind to host a vanilla faust-mode and @magnetophon or @yassinphilip could just make a new project based on this repo (or the current stand) with all the other features. I sill think @agraef is right, and sometimes just a basic behaviour that works well w/o dependencies and across updates might be still useful, and yes, we can cherrypick usefuel 'vanilla-like' features for this project.
@rukano, thanks for chiming in here. Also, I don't know whether I already thanked you for creating this mode, as you can guess it's been pretty useful for my students and me for the past few years.
So it seems that the idea of getting things fixed at least didn't get everyone up in arms right away, that's a good sign. ;-)
I've been thinking about this some more, and I do have some ideas on how we can resolve the situation so that everyone is happy.
First, those keyword lists. Don't get me wrong, I'm not against adding some support for the library functions per se. In particular, auto-completion for these would certainly be nice to have. But those lists will have to be generated dynamically (probably the easiest way to go about this is to use the installed library documentation which gets generated automatically and thus is always up-to-date). I'm tempted to work on that some time, but not right now. I can live with the static lists for now, if grudgingly, but someone (not me) will have to maintain them and keep them at least halfway in sync with current Faust. (@magnetophon maybe?)
Then there are these lines which look suspicious to me (faust-mode.el:170):
(add-to-list 'ac-modes 'faustine-mode)
(add-to-list 'ac-sources 'faustine-mode-ac-source))
I don't have auto-complete installed right now, so I didn't actually test this, but I'm pretty sure that this should be faust-mode
, not faustine-mode
there. (Talking about faustine-mode, there's a reference to that in the README as well, but the link points nowhere, as @yassinphilip's Bitbucket repository seems to have disappeared.)
I've also taken a closer look at the broken indentation, and it appears that deriving from prog-mode isn't to blame, the real culprit seems to be the improper use of SMIE. Here's how this is currently set up (at faust-mode.el:174):
(smie-setup nil #'ignore)
That just uses the defaults, which means Lisp-like syntax. No wonder that this doesn't work. If I remove this, then I can at least start typing a program without immediately getting into trouble.
Faust's syntax isn't all that esoteric. We've got the semicolon as a declaration/definition terminator, as well as curly braces and parentheses, so coming up with an SMIE grammar shouldn't be all that hard. I haven't worked with SMIE before, but if I can dig it in less than, say, 1-2 hours, I'll have a closer look and see whether I can cook up something which works. But until we have something which works this should be disabled.
That leaves me with my final gripe which I haven't mentioned yet, repo history. No offense, but I would have never let something like @yassinphilip's PR past the gate. It's riddled with trivial commits with nothing but comment changes and uninformative commit messages. This needs to be squashed, period. I don't see a single reason why we can't boil all of this down to a single commit with a descriptive commit message.
@rukano, this can't be done without rewriting history on your master branch, but I doubt that this will cause anyone trouble. I can prepare a branch on my fork which has everything as I envision it, and then you could just pull that over, make it your new master branch and be done with it. (Of course, if you want, you can also keep the old master branch for documentation purposes. But the real master branch should be kept clean.)
So, to summarize, here are my suggested changes:
squash all of @yassinphilip's PR into a single commit with a good commit message
disable the dysfunctional auto-indentation for now, until I can cook up an SMIE grammar for Faust that actually works
keep those library function lists for now and get auto-completion to work
(Yeah, I don't like the static lists, but I can understand why some people rely on them. I won't loose any sleep over this, but we should appoint someone who keeps the lists updated for now. And we should file an issue so that it's clear that this is something that has to be worked on in the future.)
fix up the README and do any other kind of final polishing needed
put some release tag on the result
I'll then update faust-mode.el in the Faust repo so that it's in the next Faust release, and @magnetophon can put it up on MELPA
no schism any more, everyone is happy (hopefully) 😃
Does that sound like a plan?
Ok, let me work on that cleaned-up branch for a bit, so that I can show you exactly what it is that I want to sell you here.
@agraef thanks and glad this all helped you and your students. :)
I would love to be more in the loop for this, but I have a full time job now, and the music and coding for fun and experiments has been reduced to a minimum.
Sure I can rename the master branch and use a newer cleaner one, as mentioned, I don't have much time to do this myself, but I can surely rename this and start a new fresh master with your PR.
@agraef Thank you very much for your constructive proposition. Let's see what @yassinphilip has to say about it.
Ok, there you go: https://github.com/agraef/emacs-faust-mode/tree/cleanup
@yassinphilip, if you happen to listen, please review my summary commit message at https://github.com/agraef/emacs-faust-mode/commit/9e3c55cf and let me know if I forgot something important.
I ran some preliminary tests with this and it appears to work all right, including auto-completion. But I still have to take it through its paces more thoroughly tomorrow, before unleashing it on the unsuspecting students. :)
@rukano, feel free to pull this over already if you want. Or you can wait until I've given it more thorough testing.
Hi guys!
As far as I remember I only PR'd small changes to faust-mode to build Faustine around. Now I just noticed (I don't do a lot of Faust dev right now) that the indentation is indeed broken again (I don't remember it being handled at all in the original faust-mode) so I'm going to look into it, I need it to work, it's a big deal. So yes, do whatever you need to be happy :)
@rukano I fully understand. But it's your baby, and people know where to find it, so I think it's preferable if we keep this as the central repo. Unless you really don't care about all this any more, that is. ;-)
If you trust me then you can also give me write access to your repo (at least temporarily) and I will pull the new branch over, rename the old master branch and make the cleaned-up and fixed version the new master branch. Note that a simple PR won't work in this case (at least I don't see how that would work), because just merging my branch into your existing master will make the history only look worse.
Also I admit having used this repo for testing my doc-a-mode package, which can explain the "trivial commits" as that was only small README.md edits.
@yassinphilip thanks for chiming in! I think that the broken indentation is just a matter of providing a proper Faust grammar to SMIE (see my remarks concerning that in the long comment above). We don't have that yet so I disabled SMIE in my branch for now, but I'll hopefully have time to look into that over the upcoming weekend. If you find a solution before me, you can send me a PR against https://github.com/agraef/emacs-faust-mode/tree/cleanup for now, until we pull that branch over to @rukano's repo.
@yassinphilip wrote:
Also I admit having used this repo for testing my doc-a-mode package, which can explain the "trivial commits" as that was only small README.md edits.
LOL. :) I cleaned that all up already in my branch, see above. Now that the indentation is fixed (well, kind of), I can actually use this and I must say that I like your improvements. The auto-completion is really useful and should also be helpful for the students. We need to write something in the README about how to install auto-complete and enable it in .emacs, though, the documentation at https://github.com/auto-complete/auto-complete is not very helpful.
@rukano got your invite, thanks.
@rukano, the cleanup branch just landed, but you'll have to do the renaming of the branches and resetting the default branch yourself, I don't seem to have permission to do that.
OK. @agraef thanks! Will do!
OK, I did some gitting around and:
cleanup
is now the new master
master
is now old-master
develop
branch for a git-flow approach, in case we keep making features, first in develop
, and then merge to master
.Should be cleaner now to fork from develop
and merge into master
for releases only.
Looking good, thanks! I have to teach courses today and tomorrow, but will have a look at the SMIE stuff over the weekend as promised.
@magnetophon, happy now?
So who's going to be responsible for keeping the function lists updated? @magnetophon, I already proposed you, are you willing to do this?
Just FYI, I archived my fork now so that anyone reading this discussion can still look at it and understand what this was all about. Will remove it as soon as we close this issue.
@agraef Great, very much appreciated!
Regarding the function list, I will definitely do what I can.
I haven't looked into what exactly it entails, so I can't really promise yet.
First I need to check how often functions are added or changed, how much work adding a function is, etc.
I'd much prefer to spend my time implementing the library lookup, and be done forever. @agraef Do you think an emacs newbie could do useful work there, with pointers from proficient elisp hackers? Does anybody know of an example of this for another language?
@rukano Could you prepare a PR to MELPA? When that is done, I'll remove my fork, and we're good for now, I think. Everybody agree?
@magnetophon sure, from my current master to where? Sorry... i'm a little out of the loop with the MELPA state.
@magnetophon can you point to the MELPA stuff, please? I have no idea how that is managed, is that a git repo?
I have no idea how that is managed, is that a git repo?
Well, it depends, it's stated anyway in the package page, and yes, in this case it's a git repo. The way this works is that once the package has been tested and approved by the MELPA team, its master branch is pulled at each roll. If said branch is tagged with a "standard" version numbering scheme, said tag is pulled at MELPA STABLE.
Well, that entry points to @magnetophon's repo, which was forked from this one but has since diverged. In fact, it's not in sync with old-master either: https://github.com/rukano/emacs-faust-mode/compare/old-master...magnetophon:master. So it's not simply a matter of sending you a PR, first we have to merge the extra changes you have that we don't and make sure that we resolve all merge conflicts.
I can have a look at that. But for the future please note that it's a fork's responsibility to keep in sync with upstream, not the other way around!
@magnetophon there you go: https://github.com/magnetophon/emacs-faust-mode/pull/4. There are some remaining merge conflicts (https://github.com/magnetophon/emacs-faust-mode/pull/4/conflicts) which you'll have to resolve by picking develop
(rather than master
) so that you have all the latest changes to faust-mode.el I made.
@rukano, the changes on our side are in https://github.com/rukano/emacs-faust-mode/tree/develop right now, as you recommended. I'll do a PR for that in a moment so that you can have a look before you merge them into master. Basically, what I did is pull over the README.md and commentary changes from faust-mode.el in @magnetophon's fork (but not the actual code; ours is newer).
For the future, I'd suggest to simply use topic branches for further development, though. I think that this is the way most people do it these days, at least once a project goes into maintenance mode and most development is done through PRs. This is more versatile and, in particular, PRs can more easily be augmented and squashed as needed -- which requires frequent rewriting of history which normally you don't want to do on a permanent development branch. (Of course, we could also use topic branches for PRs, merge these into the development branch first and then from there into master, but I'd say that this is overkill for a little project like this one.)
Many thanks to all involved! I've merged the PR, so it'll be in MELPA soon.
I think I wasn't clear when I suggested we use this repo for MELPA too. If I'm not mistaken, all active developers have access to it, right?
I've submitted a PR to make it so.
@magnetophon, +1 for making the master branch of this repo the source for MELPA.
But I'm not sure what you did there: https://github.com/rukano/emacs-faust-mode/commit/179143b13e3a8aaabcdf9a54b7051d8c03e348ac. You were supposed to merge the changes in your fork (which you did), but not the other way round. Now the history of the develop branch is the same ugly mess as downstream again. Please roll back. I took much effort so that we have a sane history here again, we don't want/need all those extra commits.
Ok, I assume that this was an accident, and I rolled back @magnetophon's changes so that develop is even with master again.
The remaining issue, if we want to make this repo work as the source of melpa stable, is tagging a release. As I squashed most of @yassinphilip's changesets into a single one, the individual commits are gone for good and we can't place any of the old release tags on them, so the best that we can do is tag a new release at this point.
The last downstream release was v0.4, so I just tagged v0.5 which now points to current HEAD on master.
@magnetophon I hope that this is good enough to make melpa happy. I checked whether there's any recent commit even in old-master that might go along with one of your release tags, but none of the commit hashes seem to match up between this repo and your fork. So I'd say that this is really the best that we can do without pulling in all of your history, which I wouldn't like to do after just having cleaned up that mess. ;-)
Just pushed the latest faust-mode.el to https://github.com/grame-cncm/faust and removed my own fork of this repo, as promised. Next up on my list is to do some more thorough testing and have a look at the SMIE stuff.
@magnetophon if we need to take additional measures concerning melpa, please let us know.
Sorry, I overlooked that one.
@magnetophon wrote:
I'd much prefer to spend my time implementing the library lookup, and be done forever. @agraef Do you think an emacs newbie could do useful work there, with pointers from proficient elisp hackers?
I'd say that this needs some halfway serious (and not just elisp) hacking. You'd have to look at how the library docs are generated in Faust. There's this faust2md script written in Python which scrapes the documentation in Markdown format from the library (.lib) files. It's hopefully possible to create a version of that script or maybe extend it to generate the function lists that we need in a format that can be used easily in elisp.
So if you know Python, how to parse Markdown in Python (or elisp) and how to juggle with files in elisp then the answer is probably yes.
Does anybody know of an example of this for another language?
It might be worth looking at the auto-complete support for other compiled languages such as C/C++ and see how they do it. (I'm almost certain, though, that these will rely on some 3rd-party tools capable of parsing C/C++ header files, which won't do you much good with Faust.)
I've also done this kind of thing for the Emacs mode of my Pure programming language, and there's surely something like that in SLIME as well. But these use dynamic interpreter environments which already have full built-in auto-completion, so all this becomes much easier than with compiled languages where all you have are the header or .lib files.
Fixed SMIE is ready: #8 Please all have a go at it and try to break it. ;-)
@yassinphilip it would be good if you could also check that the latest version in #8 still works with Faustine, too. I might give that a go some time and see whether we could use Faustine in our courses, it looks rather comprehensive.
This repo just got merged into melpa.
@yassinphilip Do you still need my fork for anything, or can I delete it?
Very nice! Since @rukano just merged the SMIE support, I'll put a v0.6 release tag on it so that it shows in melpa stable. Any complaints?
Great! Happy to see this go forward so quickly!
Well, I did find a bug in the new indentation after all. :( Try this example (from the Faust Quick Reference):
pink = f : + ~ g with {
f(x) = 0.04957526213389*x
- 0.06305581334498*x'
+ 0.01483220320740*x'';
g(x) = wrong_indent;
};
Note the wrong indent on the last line inside the with
clause. It only happens if '
is the last operator on the line above (probably an idiosyncrasy of Emacs' sexp parsing), and just changing the syntax of '
to punctuation doesn't seem to help there. So I probably need to include at least the '
operator in the SMIE grammar, or deal with that case in the indentation rules in some way.
@rukano I have some other stuff to do right now, but I'll look into this later today, so expect a bugfix PR soon. If you're happy with my work so far, then I can also just go on and merge these myself now, are you ok with that?
@agraef of course, I gave you push permissions, so feel free to pass the PR, or do it anyway for documentation purposes. :)
Thankfully, that was quite easy to fix (#9), I'll merge that later tonight when I've done a bit more testing.
Well, it wasn't so easy to fix after all. :) I spent the entire day working on the syntax table and testing stuff. But it should finally be ready to go now, will merge tomorrow if I don't hear from you.
There are still some minor issues to be looked at, but I've filed separate issues for those and will try to fix them some time next week, as time permits.
One more thing: Right now only the library functions are used with auto-complete. I'd find it convenient if all keywords and built-in functions would be in there as well. In fact, I already have that on a separate branch locally, will submit a PR in a minute. @yassinphilip was there a specific reason that you didn't just include all the keywords in ac-sources
?
All done, so I'm closing this issue.
I also fixed #10 and #11 now and tagged v0.6. I hope that I left everything in good working order, if not then yell at me. :)
Ok guys, I'm off to work on other things now (busy days, end of the semester over here), but of course I'll keep watching this space, so until the next time!
BTW, is anyone of you going to attend the first ever Faust conference which will take place at JGU (my university) in July? http://www.ifc18.uni-mainz.de/ I'd say that this would be a good opportunity to present faust-mode as well as faustine. Just sayin'. :)
is anyone of you going to attend the first ever Faust conference
I will most likely be there. I was asked to asses papers for it. :)
@agraef didn't knew about it! As I am now living in Frankfurt I might actually come! :) Gonna contact you over email if you don't mind. :)
Cool, looking forward to meet you there! :)
@rukano that's amazing, I didn't expect you to be so close! In fact I thought you were on another continent, just being a night owl like me. I'll follow up in pm later today.
@yassinphilip, do you happen to be located in the whereabouts, too? Obviously, you'd be the best candidate for presenting faust/faustine-mode at IFC, any chance you go there?
There has been some confusion over which fork to use, and now a fork by @agraef is in Faust upstream. It would be best to get some more clarity.
Since @yassinphilip is the most active (if not only) maintainer, maybe it's best if he creates a new copy, puts it in MELPA, and all others delete their fork, or at least point at his version in the readme.
That way the one that is in MELPA will not say it's a fork on the github page, and hopefully, there will be less confusion.
What do you all think?