PhoenicisOrg / scripts

Phoenicis scripts
GNU Lesser General Public License v3.0
64 stars 49 forks source link

Is the Four Level Hierarchy for Scripts required? #1058

Open madoar opened 5 years ago

madoar commented 5 years ago

I'm thinking about to merge the wine implementation and object scripts in a single script in Engines/Wine/Engine. This in turn would lead to the engine script and json file being on the same level as the application.json file.

Is this allowed/do we support this?

plata commented 5 years ago

Didn't I open an issue for this already?

madoar commented 5 years ago

I didn't search...

plata commented 5 years ago

It was https://github.com/PhoenicisOrg/scripts/issues/543

madoar commented 5 years ago

I can't find an explicit answer to my question in #543 it seems to mainly be about the conversion of the old include style to the new one.

madoar commented 5 years ago

@plata how do we continue? Do we retain the four level structure or do we want to replace it with something else?

plata commented 5 years ago

I don't like the 4 levels because it produces strange directories like "Engines/Wine/Engine" but I do not want to decide this without @qparis.

madoar commented 5 years ago

I agree that we require @qparis for a final decision, but we can only decide about something if we have a PR with a possible solution. So any ideas how to solve the issue without a four level hierarchy? :)

plata commented 5 years ago

I think this should be in #543 already but the idea is basically: do not structure the repository (and DTOs in Java) in category, application, script. Instead, manage only the scripts directly in a map (include path → DTO). To list e.g. all games do: include("applications.games.*").

madoar commented 5 years ago

Where should this Map be located? Should it be populated during the initial repository load operation or on the fly?

plata commented 5 years ago

During the load. It would replace the repository DTO hierarchy.

madoar commented 5 years ago

I don't think that we can fully replace our current DTO hierarchy. For example we still require Categories and Applications to have the applications view working correctly, otherwise it is not clear to which application a script belongs and to which category an application belongs.

plata commented 5 years ago

otherwise it is not clear to which application a script belongs and to which category an application belongs

It is because the includes are structured like applications.games.xyz, applications.development.xyz etc. Ok, you could say that this is still requiring the 4 levels inside the script ID but it's not required in the DTO.

madoar commented 5 years ago

I don't think we can remove the DTO objects and their hierarchy. What we can do in my opinion is decoupling the scripts from the DTO hierarchy. The question is then how do we detect which script belongs to which ApplicationDTO and which ApplicationDTO belongs to which CategoryDTO? Currently we detect this via the namespace hierarchy, i.e. script applications.development.notepad_plus_plus.v_7_2_2 belongs to the application applications.development.notepad_plus_plus which belongs to the category applications.development but I don't think this hierarchy fits to all types of scripts. The Wine engine implementation for example isn't really a part of an application, instead I would say it is the application.

plata commented 5 years ago

We need the namespace hierarchy only for scripts which are listed from Java. For stuff like utils, it's completely irrelevant where they are located in the hierarchy and they do not have to follow the hierarchy. If we accept that some scripts must use this hierarchy, we can get rid of the concept of ApplicationDTO and CategoryDTO and instead use a repository which consists only of ScriptDTO. In this case, we can be more flexible regarding the namespace hierarchy, e.g. the Wine implementation could be engines.wine.implementation.

madoar commented 5 years ago

So we need to add additional methods to the RepositoryDTO?

Maybe something like this:

class RepositoryDTO {
   // responsible for fetching all scripts at construction time
   // the fetched scripts are saved in a Map<String, File> object, where a script id is mapped to the file it links to
   private final ScriptFetcher scriptFetcher;

   // every application contains scripts
   List<ApplicationDTO> getApplications();

   // performs a selection of the applications based on the provided application id
   List<ApplicationDTO> getApplications(String applicationId);

   // every engine contains settings, verbs and tools
   List<EngineDTO> getEngines();

   // performs a selection of the engines based on the provided engine id
   List<EngineDTO> getEngines(String engineId);

   // the utils scripts
   List<UtilDTO> getUtils();

   // performs a selection of the utils based on the provided util id
   List<UtilDTO> getUtils(String utilId);
}
qparis commented 5 years ago

A DTO is just data class (i.e. json type). It should never have references to a code class

qparis commented 5 years ago

Sorry, I've clicked on the wrong button

madoar commented 5 years ago

Hmmm. Ok then a possible alternative is to "flatten" our DTO hierarchy. This can be done by removing the references between our DTO objects. Essentially this would reduce our DTO classes to only contain the information specified in the corresponding json files.

The references between the DTO classes (e.g. ApplicationDTO -> List<ScriptDTO> or CategoryDTO -> List<ApplicationDTO>) could then be accessed through a dedicated RepositoryFetcher class:

class RepositoryFetcher {
   public List<ApplicationDTO> fetchApplications(IdQuery category);

   public List<ScriptDTO> fetchScripts(IdQuery application);

   public List<EngineDTO> fetchEngine(IdQuery query);

   ...
}

When looking at the code snippet you will discover that I've used input parameters of type IdQuery. IdQuery could then be an interface with the following signature and implementations:

interface IdQuery {
   String getId();
}

class ApplicationDTO implements IdQuery {
   ...
}

class ScriptDTO implements IdQuery {
   ...
}

class EngineDTO implements IdQuery {
   ...
}

class UserQuery implements IdQuery {
   private final String id;

   public UserQuery(String id) {
      this.id = id;
   }

   public String getId() {
      return this.id;
   }
}

...
plata commented 5 years ago

Yes, that's what I had in mind. The thing I'm not quite sure about: Should the RepositoryFetcher know about EngineDTO etc. directly or should it only know generic scripts? In that case, a second level (ApplicationManager, EngineManager) would be responsible for the "casting".

madoar commented 5 years ago

In my example above I assumed it knows about EngineDTO, ApplicationDTO etc., therefore also the name RepositoryFetcher instead of ScriptFetcher. We could also decide to divide the responsibilities into a ScriptFetcher and a RepositoryFetcher. The issue here would be that the RepositoryFetcher would be most likely simply an extension/wrapper for the ScriptFetcher because it first fetches the scripts for a particular entity (i.e. application or engine) and then wraps them inside a corresponding object (i.e. ApplicationDTO or EngineDTO)

plata commented 5 years ago

Yes. However, that's only a small point. First, we should decide if we want continue with the hierarchy or not. In my opinion, we should remove it because it enforces an artificial structure on the scripts which makes no sense (e.g. why are Wine plugins an application?). @qparis you've started with that hierarchy. What do you think?

qparis commented 5 years ago

This is fine if you remove the structure, as long as the structure stays simple to navigate in

madoar commented 5 years ago

Now that the scripts are working again (#1076) I think we should approach this issue next so that we can finalize our script repository structure.

When we start working on this I think we also should include https://github.com/PhoenicisOrg/phoenicis/issues/1813 in the scope of this issue.

@plata what do you think?

plata commented 5 years ago

Makes sense. However, I do not find the time currently to do something about it.