Terasology icon indicating copy to clipboard operation
Terasology copied to clipboard

feature: add gooey CLI

Open pollend opened this issue 3 years ago • 15 comments

Here is an attempt to revive the gooey CLI tool. ended up cutting things down and trying to simplify things. I don't really like how the original groovy code tries to use inheritance to generalize an interface between facades, modules and libraries. I think they're diverse enough where this doesn't really make sense. I migrated in a couple of the commands which should cover most of our needs.

Original PR (with larger description): https://github.com/MovingBlocks/Terasology/pull/4065

pollend avatar Jul 01 '21 14:07 pollend

I was looking at this for formatting the diff but this is ok for the moment: https://github.com/grigoriy-orlov/git-stats/blob/master/src/main/java/ru/ares4322/gitstats/GitStatsImpl.java

pollend avatar Aug 10 '21 04:08 pollend

image

Cannot initialize Omega

DarkWeird avatar Aug 16 '21 08:08 DarkWeird

If you've decided to stick with Groovy, you may be interested to know that gradle 7 ships with Groovy 3.0.

(Related: #4653)

keturn avatar Aug 16 '21 16:08 keturn

If you've decided to stick with Groovy, you may be interested to know that gradle 7 ships with Groovy 3.0.

(Related: #4653)

that sounds really nice

pollend avatar Aug 17 '21 01:08 pollend

We should restore lib/meta/facade commands what else?

DarkWeird avatar Aug 25 '21 06:08 DarkWeird

@DarkWeird were those commands ever properly implemented?

pollend avatar Aug 30 '21 03:08 pollend

@DarkWeird were those commands ever properly implemented?

Don't sure. Groovyw just clone repos. Haven't indexes. And.. i didn't touch meta.

DarkWeird avatar Aug 30 '21 04:08 DarkWeird

I don't think we should be relying on http://meta.terasology.org/modules/list/latest can we revert it back to the recursive behavior.

pollend avatar Sep 05 '21 01:09 pollend

This is starting to look a bit overly convoluted too me. It's way to clever for its own good and I think this will be difficult to add onto in the future. I would opt to shave away some of these abstractions and separate the details more between libs,modules, and faces and keep the codebase more functional. with some helper utilities for some common things to share. this is a CLI utility, why does it need to be so complicated and a lot of this complicated logic feels unwarranted. you've tightly coupled libs, meta, module together when the details of each are not that closely bound to each other. sure there is no duplication but that is in exchange for high coupling and complexity.

I have no problem with proceeding with how this is written but I have some concessions with how this is put together at the moment.

I kind of like what @skaldarnar has written: https://github.com/skaldarnar/node-gooey

pollend avatar Sep 05 '21 01:09 pollend

is there anything else?

pollend avatar Nov 07 '21 16:11 pollend

is there anything else?

idk. :( I haven't clean and full vision.

DarkWeird avatar Nov 10 '21 08:11 DarkWeird

I did have a brief look at this and played around with it for a bit. Here are my thoughts and notes:

  • incomplete or buggy functionality
    • on running gw module init iota the following error occured:
      In copyInTemplateFiles for module ModuleTestingEnvironment - copying in a build.gradle then next checking for module.txt
      Unable to clone ModuleTestingEnvironment, Skipping: Task java.util.concurrent.FutureTask@3a419fde[Not completed, task = java.util.concurrent.Executors$RunnableAdapter@62ca1d89[Wrapped task = org.terasology.cli.commands.items.ModuleCommand$_recurseInner_closure3$_closure5@6626daa8]] rejected from java.util.concurrent.ThreadPoolExecutor@54906aa4[Shutting down, pool size = 1, active threads = 1, queued tasks = 0, completed tasks = 10] 
      
  • document the new command line interface
    • update existing documentation mentioning groovyw
    • should we put explicit documentation about this tool somewhere?
    • explain how the whole groovyw project setup works?
    • not all commands and options have descriptions
  • remove old groovyw code in a a separate PR
    • allows to work on docs in whatever time is needed
    • gives some time to keep both variants around to look out for issues
    • separate commit to remove the old code

Things I've tested:

  • init a workspace with gw module init iota
  • run a command in specific modules with gw module cmd Health Drops Inventory -c='rm build.gradle'
  • refresh faulty modules with gw module refresh
  • check the workspace status with gw workspace status
  • update outdated modules with gw module update

A few other open questions:

  • what can we do about this annoying "WARNING: An illegal reflective access operation has occurred"?
  • running a simple gw help command takes 3 seconds
  • output of workspace status is not completely clear to me, but usability can be improved in follow-ups

tl;dr: I'd like to split this PR up into smaller chunks in get it merged step by step.

skaldarnar avatar Dec 19 '21 19:12 skaldarnar

I did have a brief look at this and played around with it for a bit. Here are my thoughts and notes:

* incomplete or buggy functionality
  
  * on running `gw module init iota` the following error occured:
    ```
    In copyInTemplateFiles for module ModuleTestingEnvironment - copying in a build.gradle then next checking for module.txt
    Unable to clone ModuleTestingEnvironment, Skipping: Task java.util.concurrent.FutureTask@3a419fde[Not completed, task = java.util.concurrent.Executors$RunnableAdapter@62ca1d89[Wrapped task = org.terasology.cli.commands.items.ModuleCommand$_recurseInner_closure3$_closure5@6626daa8]] rejected from java.util.concurrent.ThreadPoolExecutor@54906aa4[Shutting down, pool size = 1, active threads = 1, queued tasks = 0, completed tasks = 10] 
    ```

* document the new command line interface
  
  * update existing documentation mentioning `groovyw`
  * should we put explicit documentation about this tool somewhere?
  * explain how the whole groovyw project setup works?
  * not all commands and options have descriptions

* remove old `groovyw` code in a a separate PR
  
  * allows to work on docs in whatever time is needed
  * gives some time to keep both variants around to look out for issues
  * separate commit to remove the old code

Things I've tested:

* init a workspace with `gw module init iota`

* run a command in specific modules with `gw module cmd Health Drops Inventory -c='rm build.gradle'`

* refresh faulty modules with `gw module refresh`

* check the workspace status with `gw workspace status`

* update outdated modules with `gw module update`

A few other open questions:

* what can we do about this annoying "WARNING: An illegal reflective access operation has occurred"?

* running a simple `gw help` command takes 3 seconds

* output of `workspace status` is not completely clear to me, but usability can be improved in follow-ups

tl;dr: I'd like to split this PR up into smaller chunks in get it merged step by step.

there is no need to split the PR its self contained so it doesn't affect the existing cli tool.

pollend avatar Dec 22 '21 16:12 pollend

[...] so it doesn't affect the existing cli tool.

Our contributor guide asks the user to run

groovyw module init iota

which does not work anymore with this PR. Thus, I don't follow the argument of this being "self-contained" and "not affecting the existing CLI tooling".

In addition, as reported above, the replacement of exactly that comment threw an error when I tried it out.

skaldarnar avatar Dec 22 '21 19:12 skaldarnar

[...] so it doesn't affect the existing cli tool.

Our contributor guide asks the user to run

groovyw module init iota

which does not work anymore with this PR. Thus, I don't follow the argument of this being "self-contained" and "not affecting the existing CLI tooling".

In addition, as reported above, the replacement of exactly that comment threw an error when I tried it out.

I'll roll back all the changes on the removed groovy code. should keep the old cli available, didn't realize I deleted it.

pollend avatar Dec 22 '21 20:12 pollend