BunsenLabs / bunsen-utilities

https://pkg.bunsenlabs.org/debian/pool/main/b/bunsen-utilities/
GNU General Public License v3.0
31 stars 21 forks source link

bl-exit 2.0 #41

Closed xaosfiftytwo closed 8 years ago

xaosfiftytwo commented 8 years ago
xaosfiftytwo commented 8 years ago

Up here for review because quite a lot has changed. Any remarks, good or bad, are gratefully received.

xaosfiftytwo commented 8 years ago

please read bin/ZZ_IMPORTANT_NOTE_TO_PACKAGER, esp. point 2, and advise.

xaosfiftytwo commented 8 years ago

Moved file ZZ_IMPORTANT_NOTE_TO_PACKAGER from bin to debian - which was my original intent.

xaosfiftytwo commented 8 years ago

I intend to merge this tomorrow, unless there are objections.

johnraff commented 8 years ago

I intend to merge this tomorrow, unless there are objections.

Actually, yes. I read in ZZ_IMPORTANT_NOTE_TO_PACKAGER that you were planning to break the backward compatibility of bl-exit's options. I don't think that's a good idea.

xaosfiftytwo commented 8 years ago

@johnraff,

Actually, it is pythons argparse module that 'invites' programmers to use a positional argument in a situation like this. No extra programming is necessary to check

  1. that exactly one argument is specified
  2. that the argument must be from a specified set of choices.

Note that the most important way of calling bl-exit - without arguments - remains unchanged.

That leaves us with 2 options:

option 1: keep backward compatibility and adjust the new version of bl-exit.

option 2: abandon backward compatability and keep the new version of bl-exit as is. We would have to coordinate the new package version with a change to .config/openbox/menu.xml and advertise the change so that users can change their menu.xml manually after the new version of bl-exit has been released.

I suppose you would favour option 1, so I am starting to check what has to be changed in the new version of bl-exit to adhere to compatibility.

So, no merge tomorrow.

No problem :)

xaosfiftytwo commented 8 years ago

@johnraff,

How do you feel about splitting bl-exit off to its own package, or to a collection of python scripts? It would allow bl-exit to be split in a python module and a short python executable script.

ghost commented 8 years ago

There's a dangling import getopt left.

I didn't have time to carefully review this but if it works reliably things should be good. I'll be testing the package in any case.

xaosfiftytwo commented 8 years ago

Now using option instead of argument for backward compatibility.

xaosfiftytwo commented 8 years ago

@2ion, by all means, go ahead. There is never too much testing of a package.

johnraff commented 8 years ago

@xaosfiftytwo I'm sorry if yesterday's post was a bit abrupt. I was in a hurry and saw your post just as I was shutting down the computer.

Thank you for preserving the GNU-style option syntax.

About splitting bl-exit off as a separate package, would it be impossible to provide the python module along with the executable inside the current bunsen-utilities package? There's no rule that everything installed by bunsen-utilities has to be an executable.

Up to now, we have divided things among packages on the basis of functionality from the point of view of the user, with the aim of optimizing modularity. bl-exit seems to belong in the same collection as bl-lock at least, even if the other scripts in bunsen-utilites are more desktop-related. (For example, bunsen-configs contains a variety of file types, because they belong together functionally.)

johnraff commented 8 years ago

@xaosfiftytwo is the python module you have in mind likely to be usable by other python scripts in the future? In that case, an alternative might be to make the module a new package (python-somemodule or whatever) and have bunsen-utilities - still containing bl-exit - depend on it.

xaosfiftytwo commented 8 years ago

@johnraff, No, it is not very likely that any part of the module will be reusable for other python scripts. The advantage of having a module is that it can be pre-compiled and optimized, and that only a minimal portion of the script is exposed to inexperienced users.

I am examining what the python rules under debian are doing in detail to see if I can just store the module in a standard python module path and pre-compile it without using the standard rules.

Anyhow, the script as it is now could be installed, and the split into script and module can be implemented later, if at all.

I would prefer a repo admin to merge the PR, and squash the commits that are now in it to a single one.

johnraff commented 8 years ago

The advantage of having a module is that it can be pre-compiled and optimized, and that only a minimal portion of the script is exposed to inexperienced users.

This sounds sensible.

I am examining what the python rules under debian are doing in detail

You've already seen these of course? https://wiki.debian.org/Python/LibraryStyleGuide https://wiki.debian.org/Python/Pybuild

Anyhow, the script as it is now could be installed, and the split into script and module can be implemented later, if at all.

OK let's go one step at a time.

I would prefer a repo admin to merge the PR, and squash the commits that are now in it to a single one.

If @Unia and @2ion are too busy, I'll have a look. It can be done locally via the cli and doesn't look too complicated.

...in fact done.

Merged, changelog updated to ver. 8.7-1 and pushed back to GitHub.

xaosfiftytwo commented 8 years ago

Thanks John

johnraff commented 8 years ago

Would you like to close the PR, since the merge has been done?