bastibe / annotate.el

Annotate.el
Other
384 stars 20 forks source link

Patched code as suggested from emacs maintainer. #122

Closed cage2 closed 2 years ago

cage2 commented 2 years ago

Hi!

This PR collect the suggestions from a kind Emacs maintainer.

Some of them are really useful (like the docstring and using some library function instead of custom code), some are a bit scary like using lexical scoping by default (i think we used dynamic scoped variables in some portion of the code, but if not i am all to switch to lexical scoping), some other are style changes that may o may not like. I am going to study all the changes because i think i need to know what the modifications do before merging.

So, in conclusion this is a code review from Emacs team! :+1:

Bye! C.

bastibe commented 2 years ago

That's a lot of modifications! They all seem reasonable at first glance, if they work as advertised. Obviously, some of them are still work in progress.

But wow, thank you and the Emacs team for helping our little library! Did you know annotate.el was featured in the latest Emacs News? That was cool to see, too (even though the presenter did not use it correctly).

cage2 commented 2 years ago

On Mon, Feb 07, 2022 at 11:42:42AM -0800, Bastian Bechtold wrote:

Hi!

That's a lot of modifications! They all seem reasonable at first glance, if they work as advertised. Obviously, some of them are still work in progress.

Unfortunately I am a bit busy at the moment, but slowly i'll start to incorporate this changes in the code splitting in multiple commit so that can be managed in a simpler way. Some of there are clearly right (like docstings), i wonder if 'remq' in better than 'cl-find', by the way. I find the second one more clear for the reader (especially newcomers) but i have not a strong opinion, remq is a nice and useful function and i learnt about it just from this patch! :)

But wow, thank you

I just sent a couple of email :)

and the Emacs team for helping our little library! Did you know annotate.el was featured in the latest Emacs News? That was cool to see, too (even though the presenter did not use it correctly).

No, this is a nice surprise! I have been happy to watch the video and read the article! I find very rewarding when your program makes someone's life easier even if just a little bit. I find that, when this happens, the time spent on the code was worth the efforts! :)

Bye! C.

cage2 commented 2 years ago

Hi @bastibe !

I think i have cleaned the code and addressed all the code marked with the label "FIXME" except one (the redundant test about checking if the buffer is using annotate-mode or not), but this should be harmless.

I was also wondered about reverting to cl-remove-if all occurrences of remq, because the name of the former is more clear to me; but i found that remq uses code implemented in C so should be faster than cl-remove-if; therefore i changed my mind and kept the remq.

in short: to me this patch can be included in the main branch. :)

Bye! C.

cage2 commented 2 years ago

Hi @bastibe !

I forgot the also flipping order in the NEWS file was suggested from Emacs people.

Bye! C.

bastibe commented 2 years ago

This all looks great! There's still a bit of commented code left that should probably be removed.

I quite enjoy doing such cleanup work. Some of my favorite tasks in the last year (at work) was going through some legacy code and getting it in order. Removing redundant abstractions, replacing outdated functions. It can be fun!

cage2 commented 2 years ago

Hi Bastian!!

This all looks great! There's still a bit of commented code left that should probably be removed.

Thanks! I was concentrating to all the work needed around the "FIXME" label and missed the rest of the commented code! 😓

I quite enjoy doing such cleanup work. Some of my favorite tasks in the last year (at work) was going through some legacy code and getting it in order. Removing redundant abstractions, replacing outdated functions. It can be fun!

To be honest i have mixed feeling when i examine the code i wrote some time before, not that i think is not fun (it is indeed, especially in this case where i learnt something new in the process of cleaning!), but because i am always a bit ashamed of the code i have written. So said i think my elisp is not so terrible, but i left a lot of "code debris" (unused variables, even unused function arguments!) and this means i lacks a bit of discipline when i program (and probably in general ;-)). Anyway i also discovered a bug when checking the code but - even if the fix is tiny- i would prefer keep this PR just for the cleaning. Also i am looking forward for the new features suggested from people on the Emacs mailing list! There is a lot of fun work to do! 😃

Bye! C.

bastibe commented 2 years ago

Oh don't worry about your discipline. Personally, I'd rather see the "debris" as a lack of elisp's tools for warning of such issues. I've been a Python programmer for a number of years now, where things like unused variables are hard to catch as well, and it's been a constant struggle. As of a year ago, however, I changed jobs and am now mostly working in C, where the compiler will warn me of these issues. In retrospect, a somewhat more stringent compiler does actually help with these sorts of issues. And I'd argue that unused variables are even harder to catch in elisp than in Python for simple syntactic clarity.

I'm looking forward to the new features! I've said it before, but annotate.el really has taken off under your stewardship! Fantastic work!

cage2 commented 2 years ago

On Wed, Feb 23, 2022 at 12:01:36AM -0800, Bastian Bechtold wrote:

Hi Bastian!

Oh don't worry about your discipline. Personally, I'd rather see the "debris" as a lack of elisp's tools for warning of such issues. I've been a Python programmer for a number of years now, where things like unused variables are hard to catch as well, and it's been a constant struggle.

Thank you for your words; yes i also rely on the compiler to check for unused variables. Likely I over criticized my work in the previous message, but anyway I'll try to check variables use carefully when writing elisp, at least until a smarter compiler will be developed. :-)

As of a year ago, however, I changed jobs and am now mostly working in C, where the compiler will warn me of these issues. In retrospect, a somewhat more stringent compiler does actually help with these sorts of issues. And I'd argue that unused variables are even harder to catch in elisp than in Python for simple syntactic clarity.

Uh nice! I like C, my experience writing code in C is quite limited but, as i lack a formal computer science education, i learnt a lot about how computer work writing in C! I hope you are getting fun with your new job!

I'm looking forward to the new features! I've said it before, but annotate.el really has taken off under your stewardship! Fantastic work!

Thanks! :) :)

I'll merge this PR tomorrow. Then I'll submit a new one to fix a minor bug. After that i think i am going to start to explore using margins; i do not know why but i have a feeling that writing in the margins could help us to (partially) get rid of an old bug that prevents annotation to show in org mode. We'll see! :)

Bye! C.

cage2 commented 2 years ago

Hi!

Merging, I just hope I have chosen the right version number. ;-) Bye! C.

bastibe commented 2 years ago

i lack a formal computer science education

Same here.

But the beauty of computer programming is that it's really not that complicated at the end of the day. No need for a university education "just" for writing programs. Computer science is a different matter, I suppose. But what we're doing is hardly scientific, lacking in rigor and theoretical proofs. Being a scientist and engineer, I'd even say most programming isn't even engineering, what with the lack of procedures and stringent reviews. And thank goodness for that, as I wouldn't want to drive over a bridge built as shoddily as, say, Microsoft Word.

bastibe commented 2 years ago

I also teach computer programming (not computer science/engineering) to undergrads. It's kind of amazing how quickly they can learn how to program. Few other topics can be approached as easily in university.

cage2 commented 2 years ago

On Wed, Mar 02, 2022 at 12:38:08AM -0800, Bastian Bechtold wrote:

i lack a formal computer science education

Same here.

I was starting to attend some courses but unfortunately the pandemic (an my personal situation) prevented me to do so. :(

But the beauty of computer programming is that it's really not that complicated at the end of the day.

I agree! Also for people as old as me that grow up in the microcomputer era (commodore 64 etc.) thinking that the games you were playing was written by a single person using the same hardware was very inspiring to start programming! :)

I'd even say most programming isn't even engineering,

In a sense is also a way of express creativity, if not art, someway.

And thank goodness for that, as I wouldn't want to drive over a bridge built as shoddily as, say, Microsoft Word.

:D :D :D

Bye! C.

cage2 commented 2 years ago

On Wed, Mar 02, 2022 at 12:39:19AM -0800, Bastian Bechtold wrote:

Hi!

I also teach computer programming (not computer science/engineering) to undergrads. It's kind of amazing how quickly they can learn how to program. Few other topics can be approached as easily in university.

Nice! Which languages do you teach? Do you think are persons getting smarter or modern programming languages that makes programming easier? Or both things or neither of the two? :)

Bye! C.

bastibe commented 2 years ago

I used to teach Matlab, but have converted the syllabus to Python over the years, mostly focusing on real-world problems such as how to deal with messy data, how to organize a non-trivial project, and introducing students to common technologies for various engineering tasks.

We've had many a heated discussion in the faculty on the topic of programming languages appropriate for learning. From my own experience, I can say that the students found it helpful if their first language put as few obstacles in their way as possible.

Python works well for the purpose, with problems usually centering around how to install it, what editor to use, what is an editor, how does the command line work, etc. But once students are set up with a working environment, they can usually get started with programming relatively easily.

In general, I find these environmental issues to be the most perplexing to students. During their studies at our university, they may see Java, Matlab, C, and Python. Java and Matlab come with an integrated IDE, which makes simple things simple, and hard things impossible (for students). C was taught from the command line only. Which is painful for sure, but seems to leave students better-prepared for eventual problems. Python seems to cause little trouble if used from the command line, but can be very confusing to work with from an IDE (Visual Studio Code, usually), due to conflicts between the IDE-provided interpreter and packages, and externally installed ones. These are typical Python problems.

Nevertheless, approachable tools such as Visual Studio Code and Python do help the students to get started more easily. Modern web-based documentation and youtube tutorials also seem to help a lot. In these areas, modern students have it easy. On the other hand, we see a tremendous lack in basic computer knowledge. Students nowadays often have only a dim understanding of files and directories, preferring to do everything "in an app". This makes it much harder to teach concepts such as command lines.

Well, and Covid was truly a tragedy for students. Learning to listen and learn in a classroom setting is difficult enough. But doing so at home in front of a computer screen, without physical interactions with other students is beyond my capacity to comprehend.