Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is the Four Level Hierarchy for Scripts required? #1058

Open
madoar opened this issue Jul 2, 2019 · 24 comments
Open

Is the Four Level Hierarchy for Scripts required? #1058

madoar opened this issue Jul 2, 2019 · 24 comments
Assignees

Comments

@madoar
Copy link
Collaborator

madoar commented Jul 2, 2019

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
Copy link
Collaborator

plata commented Jul 2, 2019

Didn't I open an issue for this already?

@madoar
Copy link
Collaborator Author

madoar commented Jul 2, 2019

I didn't search...

@plata
Copy link
Collaborator

plata commented Jul 3, 2019

It was #543

@madoar
Copy link
Collaborator Author

madoar commented Jul 3, 2019

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
Copy link
Collaborator Author

madoar commented Jul 7, 2019

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

@plata
Copy link
Collaborator

plata commented Jul 7, 2019

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
Copy link
Collaborator Author

madoar commented Jul 7, 2019

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
Copy link
Collaborator

plata commented Jul 7, 2019

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
Copy link
Collaborator Author

madoar commented Jul 7, 2019

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

@plata
Copy link
Collaborator

plata commented Jul 8, 2019

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

@madoar
Copy link
Collaborator Author

madoar commented Jul 9, 2019

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
Copy link
Collaborator

plata commented Jul 9, 2019

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
Copy link
Collaborator Author

madoar commented Jul 9, 2019

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
Copy link
Collaborator

plata commented Jul 10, 2019

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
Copy link
Collaborator Author

madoar commented Jul 10, 2019

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
Copy link
Member

qparis commented Jul 11, 2019

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

@qparis qparis closed this as completed Jul 11, 2019
@qparis qparis reopened this Jul 11, 2019
@qparis
Copy link
Member

qparis commented Jul 11, 2019

Sorry, I've clicked on the wrong button

@madoar
Copy link
Collaborator Author

madoar commented Jul 11, 2019

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
Copy link
Collaborator

plata commented Jul 26, 2019

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
Copy link
Collaborator Author

madoar commented Jul 27, 2019

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
Copy link
Collaborator

plata commented Jul 27, 2019

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
Copy link
Member

qparis commented Aug 6, 2019

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

@madoar
Copy link
Collaborator Author

madoar commented Aug 11, 2019

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 PhoenicisOrg/phoenicis#1813 in the scope of this issue.

@plata what do you think?

@plata
Copy link
Collaborator

plata commented Aug 12, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants