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

Fixed: path-must-specify-tags #40

Merged
merged 6 commits into from
Apr 30, 2024
Merged

Conversation

LucasMaciuga
Copy link
Contributor

Hi,

I run into a bug in the path-must-specify-tags rule. If you define under schemas a model with a property that is named "get,post,put,delete,patch,options,head or trace" it will raise an error/warning.

I fixed it, by removing the 'dot' in the JSONPath under "given".
I wrote an additional test which will be raised when the dot is present in the given-selector.

Please check if the selector is correct - tested the selector with https://jsonpath.com/, but it gives me not the same result as the implementation of spectral.

@@ -60,4 +65,115 @@ describe("path-must-specify-tags", () => {
},
},
});

const getComponentTestSpec = (tags?: string[], path = "/api/some/path") =>
JSON.stringify({
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's neccessary to have the full spec for this unit test. Only a path and a component should be neccessary to reproduce the issue.

.gitignore Outdated
@@ -1,6 +1,7 @@
.vscode
demo*
node_modules
pnpm-lock.yaml
Copy link
Member

Choose a reason for hiding this comment

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

This repo is currenly using npm, please don't add pnpm related changes in this PR. However if you'd like to migrate this project to pnpm I'd be greatful for a seprate PR ;)

package.json Outdated
@@ -5,6 +5,7 @@
"license": "Apache-2.0",
"devDependencies": {
"@stoplight/spectral-core": "^1.18.3",
"@stoplight/spectral-runtime": "^1.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

Why was this dependency added? I guess this has something to do with the exclusion of the pnpm lockfile from git. Whenever a dependency is changed the lockfile should be updated as well which isn't the case here. Please use npm when working within this project to not corrupt the existing package-lock.json.

@markbrockhoff markbrockhoff merged commit 68d460e into SchwarzIT:main Apr 30, 2024
2 checks passed
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

2 participants