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 [semver.minor]: allow css configuration from package.json #1499

Conversation

jeswr
Copy link
Contributor

@jeswr jeswr commented Nov 4, 2022

πŸ“ Related issues

closes #1465

✍️ Description

βœ… PR check list

Before this pull request can be merged, a core maintainer will check whether

  • this PR is labeled with the correct semver label
    • semver.patch: Backwards compatible bug fixes.
    • semver.minor: Backwards compatible feature additions.
    • semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
  • the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
  • the RELEASE_NOTES.md document in case of relevant feature or config changes.
  • any relevant documentation was updated to reflect the changes in this PR.

@jeswr jeswr marked this pull request as ready for review November 4, 2022 14:14
@jeswr
Copy link
Contributor Author

jeswr commented Nov 5, 2022

I'm not sure I understand where the failure in the conformance test is coming from as it seems to be unrelated to the code that I touched. For reference here is an example of 1 of the 3 errors that occurs.

2022-11-05T02:56:30.0746928Z 2022-11-05 02:56:30,067 ERROR [com.int.karate] (main) ../data/web-access-control/protected-operation/write-access-public.feature:100
2022-11-05T02:56:30.0747469Z Then match [401] contains responseStatus
2022-11-05T02:56:30.0747792Z match failed: CONTAINS
2022-11-05T02:56:30.0748421Z   $ | actual does not contain expected | actual array does not contain expected item - 404 (LIST:NUMBER)
2022-11-05T02:56:30.0748781Z   [401]
2022-11-05T02:56:30.0748983Z   404
2022-11-05T02:56:30.0749116Z 
2022-11-05T02:56:30.0749272Z     $[0] | not equal (NUMBER:NUMBER)
2022-11-05T02:56:30.0749535Z     401
2022-11-05T02:56:30.0749731Z     404
2022-11-05T02:56:30.0749861Z 

@joachimvh
Copy link
Member

I'm not sure I understand where the failure in the conformance test is coming from as it seems to be unrelated to the code that I touched.

This is an issue that has been fixed in the main branch but has not yet been merged into the 6.0.0 branch.

@joachimvh
Copy link
Member

I only noticed just now that you marked this PR as ready for review some time ago, but then you also did some commits after that. So is this ready to look at?

@jeswr
Copy link
Contributor Author

jeswr commented Nov 21, 2022

@joachimvh - yes it is ready now :)

Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

You can request me as reviewer in the PR for the next round, that way chances are lower I miss it.

This PR targets the 6.0.0 branch but I see no reason why it can't target the main branch though. This only extends functionality and doesn't break anything existing.

.eslintrc.js Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
documentation/markdown/README.md Outdated Show resolved Hide resolved
documentation/markdown/usage/dev-configuration.md Outdated Show resolved Hide resolved
documentation/markdown/usage/dev-configuration.md Outdated Show resolved Hide resolved
src/init/AppRunner.ts Outdated Show resolved Hide resolved
src/init/AppRunner.ts Outdated Show resolved Hide resolved
test/unit/init/PackageJsonInitializer.test.ts Outdated Show resolved Hide resolved
test/unit/init/PackageJsonInitializer.test.ts Outdated Show resolved Hide resolved
test/unit/init/PackageJsonInitializer.test.ts Outdated Show resolved Hide resolved
@jeswr jeswr force-pushed the feat/init-from-package-json branch from 40b6bf2 to 04986ab Compare November 22, 2022 05:12
@jeswr jeswr requested a review from joachimvh November 22, 2022 06:14
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor fixes.

Want to repeat that this can target the main branch in my opinion (although it doesn't have to if you prefer this).

RELEASE_NOTES.md Outdated Show resolved Hide resolved
documentation/markdown/usage/dev-configuration.md Outdated Show resolved Hide resolved
src/init/AppRunner.ts Outdated Show resolved Hide resolved
src/init/AppRunner.ts Outdated Show resolved Hide resolved
src/init/AppRunner.ts Outdated Show resolved Hide resolved
src/init/AppRunner.ts Outdated Show resolved Hide resolved
src/init/AppRunner.ts Outdated Show resolved Hide resolved
jeswr and others added 8 commits November 23, 2022 19:13
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
@jeswr jeswr requested a review from joachimvh November 23, 2022 08:25
@jeswr
Copy link
Contributor Author

jeswr commented Nov 23, 2022

Just realised I forgot to rebase to main - doing that now

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