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

feat(spec): tag POC #435

Merged
merged 14 commits into from
Apr 27, 2022
Merged

feat(spec): tag POC #435

merged 14 commits into from
Apr 27, 2022

Conversation

bodinsamuel
Copy link
Contributor

@bodinsamuel bodinsamuel commented Apr 26, 2022

🧭 What and Why

🎟 JIRA Ticket:

Changes included:

  • POC: tagging request to group them.
    It's mostly useful for frontend but could be reuse to filter requests and build lighter version automatically.

Seems to work fine and not break anything.
Let me know what you think.

🧪 Test

current after
Screenshot 2022-04-26 at 19 33 39 Screenshot 2022-04-26 at 19 33 48

@bodinsamuel bodinsamuel self-assigned this Apr 26, 2022
@netlify
Copy link

netlify bot commented Apr 26, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit de0c17a
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/62696a618a23a30008d455ff

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 26, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

@bodinsamuel bodinsamuel changed the title feat(all): tag POC feat(spec): tag POC Apr 26, 2022
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

That's a good idea :D Waiting for the CI to see how many things broke :(

Also, you can assign this ticket as it's its goal: https://algolia.atlassian.net/browse/APIC-268

specs/search/spec.yml Show resolved Hide resolved
scripts/buildSpecs.ts Show resolved Hide resolved
@millotp
Copy link
Collaborator

millotp commented Apr 26, 2022

Good idea ! There seems to be some tag left that generate vaultClient, ApiKeysClient... and is likely to break. It might be possible using custom property like x-tag`.

@bodinsamuel
Copy link
Contributor Author

bodinsamuel commented Apr 26, 2022

Yep the tags property is doomed. I Generated 2 files. However it seems to break netlify build, not sure why yet

scripts/buildSpecs.ts Outdated Show resolved Hide resolved
website/docusaurus.config.js Show resolved Hide resolved
netlify.toml Outdated Show resolved Hide resolved
@bodinsamuel bodinsamuel marked this pull request as draft April 27, 2022 11:12
@bodinsamuel
Copy link
Contributor Author

Okay it works https://deploy-preview-435--api-clients-automation.netlify.app/specs/search
I had to rework a bit the bundling script, looks great for a poc imo

scripts/buildSpecs.ts Outdated Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

That looks so good thank you !

@@ -47,3 +49,27 @@ html[data-theme='dark'] .header-github-link:before {
background: url("data:image/svg+xml,%3Csvg viewBox='0 0 24 24' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath fill='white' d='M12 .297c-6.63 0-12 5.373-12 12 0 5.303 3.438 9.8 8.205 11.385.6.113.82-.258.82-.577 0-.285-.01-1.04-.015-2.04-3.338.724-4.042-1.61-4.042-1.61C4.422 18.07 3.633 17.7 3.633 17.7c-1.087-.744.084-.729.084-.729 1.205.084 1.838 1.236 1.838 1.236 1.07 1.835 2.809 1.305 3.495.998.108-.776.417-1.305.76-1.605-2.665-.3-5.466-1.332-5.466-5.93 0-1.31.465-2.38 1.235-3.22-.135-.303-.54-1.523.105-3.176 0 0 1.005-.322 3.3 1.23.96-.267 1.98-.399 3-.405 1.02.006 2.04.138 3 .405 2.28-1.552 3.285-1.23 3.285-1.23.645 1.653.24 2.873.12 3.176.765.84 1.23 1.91 1.23 3.22 0 4.61-2.805 5.625-5.475 5.92.42.36.81 1.096.81 2.22 0 1.606-.015 2.896-.015 3.286 0 .315.21.69.825.57C20.565 22.092 24 17.592 24 12.297c0-6.627-5.373-12-12-12'/%3E%3C/svg%3E")
no-repeat;
}

/* Font */
@import url("https://fonts.googleapis.com/css?family=Open+Sans");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

for a poc I guess it's fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use it on the crawler and for a POC not hosted on algolia for the moment I would not bother much

for (const pathMethods of Object.values(bundledSpec.paths)) {
for (const specMethod of Object.values(pathMethods)) {
specMethod.tags = [client];
let bundledDocSpec: Spec | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why load the yaml twice ? Can you simply copy the file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem in JS is pointer ref I can not simply reuse the same object.
So either I copy the file first and load it
or load the same file twice. which is equivalent I would say. No strong opinion

scripts/buildSpecs.ts Outdated Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
@bodinsamuel bodinsamuel marked this pull request as ready for review April 27, 2022 14:32
@bodinsamuel
Copy link
Contributor Author

I guess next steps is to add more tags to all specs

scripts/buildSpecs.ts Outdated Show resolved Hide resolved
continue;
}

// Checks that specified tags are well defined at root level
Copy link
Member

Choose a reason for hiding this comment

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

I there a reason we want to ensure 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.

because if you don't define tags at the root level then nothing will appear in the doc

Copy link
Member

Choose a reason for hiding this comment

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

It will still default to the base tag so I guess it's fine, what I mean is this feature is for the doc and shouldn't prevent dev but it's just my opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once it's done it's done, if you add a tag that means you expect to change the doc. So I'm not too worried about this impacting dev flow.

Comment on lines +2 to +3
tags:
- Indices
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to avoid any manual changes added in the specs, and default to the name of the folder the spec is stored in?

This way, we only have to handle where to put the file, not what tag it should match

Copy link
Member

Choose a reason for hiding this comment

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

So with today's structure, the tag for getTask would be advanced, but we can also create a new folder for indices, and we default to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope because of deleteIndex and saveObject living in the same rest path

Copy link
Member

@shortcuts shortcuts Apr 27, 2022

Choose a reason for hiding this comment

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

Yeah sorry I did not fully explained why, but looking at the rest path, search settings objects all have the same so I guess we can do whatever we want here with the folders, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could but it would need more care in spec.yml, experience tells me deducting something from the filesystem will blow up on your face at some point.
And deleteIndex and saveObject are not splittable because of the limitation of $ref object, so my point still stand.

Copy link
Member

@shortcuts shortcuts Apr 27, 2022

Choose a reason for hiding this comment

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

They are both index operations so it wouldn't choc me to see both of them under an index folder/tag.

My point is only that we don't have extra logic but I get yours too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one is index operation, the other record operation. it's definitely debatable but as a user I would find it weird :p

Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

GG !

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

(Nothing else to add if you don't go with the tag by folder thing)

Really nice addition :D

@bodinsamuel
Copy link
Contributor Author

bodinsamuel commented Apr 27, 2022

(Nothing else to add if you don't go with the tag by folder thing)

Still debatable anyway, we'll maybe see with the others spec what could be improved ☺️

@bodinsamuel
Copy link
Contributor Author

I can not merge 🤣

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.

4 participants