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

Fix apiGroup sorting (#1042) #1044

Merged
merged 4 commits into from
Oct 18, 2021
Merged

Fix apiGroup sorting (#1042) #1044

merged 4 commits into from
Oct 18, 2021

Conversation

esaracco
Copy link
Contributor

No description provided.

@esaracco
Copy link
Contributor Author

@NicolasCARPi This first push fixes the sort in case order contains apiGroup. However, there is still a issue when it contains only apiName. I am on it.

@esaracco
Copy link
Contributor Author

@NicolasCARPi I may be wrong, but I see a difficulty related to the fact that we can mix api-names and api-groups in the order section. What should be the priority to give to each other? Wouldn't it be relevant to create two dedicated order sections? One for api-groups and the other for api-names? What was the primary purpose of this section?

@esaracco
Copy link
Contributor Author

@NicolasCARPi For instance, my PR succeeds in sorting with the following configurations:

 "order": [
    "01 - Setup",
    "setup.findOne",
    "setup.update",

    "02 - Accounts",
    "accounts.findAll",
    "accounts.findOne",
    "accounts.create",
    "accounts.delete",
    "accounts.update",

    "99 - Misc",
    "misc.findAllSomething",
    "misc.findAllSomethingElse",
    "misc.deleteSomething",
    "misc.changeSomething"
]

or even:

 "order": [
    "01 - Setup",
    "02 - Accounts",
    "99 - Misc",

    "setup.findOne",
    "setup.update",

    "accounts.findAll",
    "accounts.findOne",
    "accounts.create",
    "accounts.delete",
    "accounts.update",

    "misc.findAllSomething",
    "misc.findAllSomethingElse",
    "misc.deleteSomething",
    "misc.changeSomething"
]

This way, both api-groups and api-names are correctly sorted.
In either case, you need to explicitly specify the order of the api-groups.
Is this behaviour OK for you?

@NicolasCARPi
Copy link
Collaborator

I see a difficulty related to the fact that we can mix api-names and api-groups in the order section.

Yes I see it too!

What should be the priority to give to each other?

¯\_(ツ)_/¯

Wouldn't it be relevant to create two dedicated order sections? One for api-groups and the other for api-names?

That was also what I thought.

What was the primary purpose of this section?

Allow ordering of components of course! If your code does what you say it does, I think it's very good already and we don't need to have two ordering keys.

@pilotkid
Copy link

pilotkid commented Oct 17, 2021

It worked for me.

 "order": [
    "setup.findOne",
    "setup.update",

    "accounts.findAll",
    "accounts.findOne",
    "accounts.create",
    "accounts.delete",
    "accounts.update",

    "misc.findAllSomething",
    "misc.findAllSomethingElse",
    "misc.deleteSomething",
    "misc.changeSomething",

    "01 - Setup",
    "02 - Accounts",
    "99 - Misc"
]

If I may chime in to the discussion about the ordering field. I did find it very confusing when I first started using apidoc on how to sort the groups and the calls themselves.

Personally how I feel it should be structured is having order become an object, with sub-objects with the keys being the groups and the value being an array in the order in which to sort.

 "order": {
    "01 - Setup": [
        "setup.findOne",
        "setup.update",
     ],

    "02 - Accounts":[
        "accounts.findAll",
        "accounts.findOne",
        "accounts.create",
        "accounts.delete",
        "accounts.update",
    ],

    "99 - Misc":[
        "misc.findAllSomething",
        "misc.findAllSomethingElse",
        "misc.deleteSomething",
        "misc.changeSomething",
    ]
}

This would also allow for backward compatibility. You would just have to check to see if the order is an object or an array.

I would be happy to see if I can accomplish this if I get the green light from @NicolasCARPi

@NicolasCARPi
Copy link
Collaborator

@pilotkid Yes, what you suggest looks like a good idea to me!

@NicolasCARPi NicolasCARPi merged commit 78efc2a into apidoc:dev Oct 18, 2021
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.

None yet

3 participants