Remove xmlsitemap_helper.module, and move the code to xmlsitemap.module

I've looked at the code, and I don't understand the intent of the 'helper' either.
Overall, it made the code harder to analyze, less pleasant to work with, and is probably one of the reasons I can't assist fixing the current issues, although I'd really like to help.

If there was logic, a roadmap, or any overview on what parts of the code to what, it would help. At the moment I just feel I'm missing something when I look at the pieces.

The reason of having two modules is that one implements the user interface, and one implements the functions used by other modules. I don't see any complaning about other modules being splitted in two.

There is a misunderstanding: the interface module is xmlsitemap.module.
When the code will be complete, it is xmlsitemap_helper.module to create the main database table. I know, the choice of the name has not been perfect, but I am not turning back to have a single module.

Talking of modules, I find rather strange to have a module for the node links, and one for the node attachment links, with the effect of duplicate code and executed SQL queries. It would be better to have a single module to handle the node links.

Separating the core library, the UI, and possibly an API into discrete bits is often a good idea. It can help maintenance, modularity and extensibility. If there is a clear distinction and the parts are pluggable.
But which is which?
I can't tell if 'helper' contains the supporting libraries (it appears to be required) or if 'helper' is supposed to be the optional user interface.

cross-post, sorry.

See my previous comment. I understand the name I chose is not perfect, but I was also trying to not introduce too much new module names.

The module has then been introduced to resolve an issue some of the XML Sitemap users were having. xmlsitemap.module has been moved into a different directory so that it is possible to create the translation files using Translation template extractor. This caused some Drupal installations to report errors about not existing functions; the solution to the problem has been to create a new module (xmlsitemap_helper.module).

I meant that I moved all in a sub-directory because it's easier to create the translation files when the code of a module is spitted on more files.

I will rename the modules in the 6.x-2 branch.

This sounds like a good change to make. It's not critical, but it would be more inline with other modules (Views/Views UI for example) and make the module a bit easier to parse for new contributors.

Hm. Though actually, if we're doing an API/UI split, there's a lot of code that needs to be switched around. For example:
- The main database table is contained in xmlsitemap module, not xmlsitemap_helper module.
- All of the functions to do things like refresh the sitemap are in xmlsitemap module, not xmlsitemap_helper module. (xmlsitemap_helper_update_sitemap would lead you to believe that it updates the sitemap, but it does not.)
- xmlsitemap_helper also contains some UI stuff; for example xmlsitemap_helper_user_form_fieldset().

As such I'm left scratching my head as well about the reason for this module's existence. Separating API from UI stuff is worthy goal, particularly if the UI is really heavy. But this seems more like a "stick any function that's called from more than one place in a separate module" kind of move, and I'm not sure that's really helpful. There is also a fair amount of UI code in the other modules (_taxonomy, _node, _menu, etc.) which can't be disabled without losing their functionality.

Am I missing something?

To talk of what the actual situation is, xmlsitemap_helper.module contains functions called from more than one modules; it's a kind of repository of public functions for the other modules (even third-party).

What I reported here was my thought about the future development of that module, which I now think it's an idea that is not possible to realize.

Talking of modules that were not strictly necessary, I would include xmlsitemap_engines.module, which could be incorporated in xmlsitemap.module; also xmlsitemap_file.module can be incorporated in an existing module.

Some of us have our terminology backwards, I'm not sure who.

The name {modulename}_helper implies it's totally optional, and not required for anything - just a bonus pack. It looks a bit like {modulename}_ui which tells me it's not needed, but you enable it if you want to poke around, and can be removed after you've finished.

{modulename}_api may describe that this is a library that allows integration with third parties. This may be the direction to go.
{modulename}.module should be self-sufficient, and may publish some public methods, unless it in turn makes use of libraries in {modulename}_api - in which case it can require the *_api - but only if the API in turn can stand on its own and continue to be called by others after the {modulename}.module is gone.
Dependencies are one way. We should avoid co-dependency. otherwise that illustrates that the libraries need refactoring.

The required library functions should not be called _helper.
IMO.

Build the core libraries, and anything that needs to be shared in one place {modulename}. Then stick the UI, admin over top of that if needed {modulename}_ui . With the idea that another ui may also be capable of administering parts, or another engine may leverage the library.

... I reckon anyway. See imagecache/imageapi or views/views_ui for examples.
imagecache desperately requires imageapi, but put it in a library so others can get at it too.
views does not need views_ui at all, as it can operate without it.
That is modularization done right.

xmlsitemap_helper.module has been a bad idea, and a completely wrong way to resolve an issue somebody was having.
It's clear that the only purpose of the module is to contain a number of functions used by more than one module; using it for any other purposes seems something pushed just to find a reason for the module to exist.

I think that I should better delete the module, and move the functions to xmlsitemap.module.

@Kiam: One idea if you're interested in separating out the UI from the "do stuff" code without introducing extra dependencies is to do page splitting like core does in most of its modules. For example, you could put all of the admin-facing UI code for xmlsitemap.module in a xmlsitemap.admin.inc file, and the admin-facing UI code for xmlsitemap_engines.module in xmlsitemap_engines.admin.inc. Then just change the menu item definition in hook_menu() so that it has a 'file' index. See http://api.drupal.org/api/function/upload_menu for an example.

This way, the UI code that's in that file will only be loaded if you're actually on a menu path that explicitly includes it. This is a new handy feature of the 6.x menu system.

I am changing the task.

The functions have been moved into xmlsitemap.module. The content of the xmlsitemap_helper directory needs to be removed, and the modules dependency must be updated.

I am setting the report as fixed.

xmlsitemap_helper.module is completely gone in the Drupal 6 branch. It must be removed in the Drupal 5 branch code too.

The changes have been committed in CVS.