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

Add categories on json list #149

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

danielebarbaro
Copy link
Contributor

@danielebarbaro danielebarbaro commented Oct 1, 2020

Hi @mattstauffer , first implementation of #143 could be like this:

Screenshot 2020-10-01 at 17 30 16

The json is:

{
  "Cache": [
    [
      {
        "container_id": "3aac7ef4477f",
        "names": "TO--memcached--1.6.7",
        "status": "Up 7 hours",
        "ports": "0.0.0.0:11211->11211\\/tcp"
      }
    ]
  ],
  "Database": [
    [
      {
        "container_id": "4dc197902954",
        "names": "TO--mariadb--focal",
        "status": "Up 7 hours",
        "ports": "0.0.0.0:3342->3306\\/tcp"
      }
    ],
    [
      {
        "container_id": "0daed47e14a5",
        "names": "TO--postgresql--13.0",
        "status": "Up 3 days",
        "ports": "0.0.0.0:5647->5432\\/tcp"
      }
    ]
  ],
  "Mail": [
    [
      {
        "container_id": "202e824629b1",
        "names": "TO--mailhog--v1.0.1",
        "status": "Up 7 hours",
        "ports": "0.0.0.0:8025->8025\\/tcp, 0.0.0.0:2341->1025\\/tcp"
      }
    ]
  ]
}

I decided to group container by category as i've done in the principal menu #88 (PR #135) but we can discuss about it.

I think that isn't useful category column in list output.

@danielebarbaro
Copy link
Contributor Author

Hi @mattstauffer now PR is updated.
All tests pass.

This PR needs #157 .

@danielebarbaro
Copy link
Contributor Author

🥺

@mattstauffer
Copy link
Member

@danielebarbaro Whoops, sorry!

  1. I think I'd prefer we use the lowercase version of the categories, but I'd be up to someone telling me the uppercase is better.
  2. My natural inclination would be just to add it to each item in the list rather than grouping them this way. @mpociot as the first person who's talked about possibly consuming this data, what do you think?

@danielebarbaro
Copy link
Contributor Author

hi @mpociot , what you think about this implementation?

I think I'd prefer we use the lowercase version of the categories, but I'd be up to someone telling me the uppercase is better.

oookkeeeei 🧐 🤓

My natural inclination would be just to add it to each item in the list rather than grouping them this way. @mpociot as the first person who's talked about possibly consuming this data, what do you think?

I prefer grouping to "select" only one group a time 💪🏻

@mattstauffer
Copy link
Member

@danielebarbaro OK, I'm going to say definitely throw them into the main body of each entry, and give a flat response of entries.

@josecanhelp had a great idea as well: To add the category under a meta key, so it's separated from the rest.

Thanks!

@danielebarbaro
Copy link
Contributor Author

Cool, if you need any help i can support you .
I noticed your last PR merges and i saw Base Alias && Full Alias , well done @josecanhelp 🥇

@mattstauffer
Copy link
Member

@danielebarbaro If you're up to code the change, I'd love it! If you don't have time, please let me know.

@danielebarbaro
Copy link
Contributor Author

@danielebarbaro If you're up to code the change, I'd love it! If you don't have time, please let me know.

i'm always on the cutting edge 🏄🏼‍♂️. i will rewrite the PR with the new changes

@danielebarbaro
Copy link
Contributor Author

danielebarbaro commented Nov 10, 2020

Hi @mattstauffer , PR updated but i have some doubts to discuss.

Screenshot 2020-11-10 at 15 49 26

  • takeout list --json have different output from takeout list is it what we want?

My opinion is to have same output (add category column on list) and with #173 filter base alias, full alias and category fields
Maybe category can be always present 🧐

  • Full Alias and Base Alias are lowercase, category isn't.

My opinion is to have same output :) convert category to lowercase and use ucfirst() on menu (takeout enable).

  • behind the scenes i use container name to get the right category however, i would like to use base_alias

My opinion is: container name isn't clean to parse, base alias is "more confident" to use

@danielebarbaro
Copy link
Contributor Author

🙋🏻‍♂️

@mattstauffer
Copy link
Member

takeout list --json have different output from takeout list is it what we want?
Yep!

Full Alias and Base Alias are lowercase, category isn't.
I agree, let's strtolower the category

behind the scenes i use container name to get the right category however, i would like to use base_alias
Makes sense!

app/Services/Category.php Outdated Show resolved Hide resolved
@@ -10,4 +12,30 @@ abstract class Category
const SEARCH = 'Search';
const STORAGE = 'Storage';
const TOOLS = 'Tools';

public static function all(): array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would more be a method on the Service class named allByCategory or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all() now returns:

$result = [
    'beanstalkd' => "Cache",
    'clickhouse' => "Database",
    'dynamodb' => "Cache",
    'elasticsearch' => "Search",
    'eventstoredb' => "Database",
    'expose' => "Tools",
    'influxdb' => "Database",
    'mailhog' => "Mail",
    'mariadb' => "Database",
    'meilisearch' => "Search",
    'memcached' => "Cache",
    'minio' => "Storage",
    'mongo' => "Database",
    'mssql' => "Database",
    'mysql' => "Database",
    'neo4j' => "Database",
    'postgis' => "Database",
    'postgresql' => "Database",
    'redis' => "Cache",
    'sftp' => "Storage",
    'sqs' => "Cache"
];

so the right name (for me) is mapByService()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I still think this is more of a list of services than a list of categories, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops sorry 🙈 I misunderstood!
Apologies, you're right, it is a method for Service class.

@mattstauffer
Copy link
Member

@danielebarbaro Thanks for all your hard work on this!

@mattstauffer
Copy link
Member

@danielebarbaro OK, I'm finally getting back into my open source tasks. Taking a look now!

@@ -2,6 +2,8 @@

namespace App\Services;

use App\Services;

abstract class Category
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this has been sitting open for ages so I'm sorry to make one more request, but could you write tests for these two new methods?

Thanks so much!

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

Successfully merging this pull request may close these issues.

2 participants