[CosmosDB] formatting

[deyaaeldeen]

The result of running rush format.

[southpolesteve]

Cosmos already runs prettier and verifies it in CI. What's the plan for larger repo wide formatting? How will it be enforced?

I am a little surprised to see the actual difference here. And that the tests still pass. I would expect them to fail since the formatting changed.

[deyaaeldeen]

I would suggest to verify that it works in the CI. It fails on master.

dalmahal@LAPTOP-FT6JHU6R:~/repo2/sdk/cosmosdb/cosmos$ git branch -v
* master               1043492c3 Containerinstance release 6 (#10253)

dalmahal@LAPTOP-FT6JHU6R:~/repo2/sdk/cosmosdb/cosmos$ rushx check-format
Found configuration in /home/dalmahal/repo2/rush.json


Rush Multi-Project Build Tool 5.28.0 - https://rushjs.io
Node.js version is 14.5.0 (pre-LTS)

Found configuration in /home/dalmahal/repo2/rush.json

Executing: "prettier --list-different --config ../../.prettierrc.json --ignore-path ../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\""

src/client/Item/Item.ts
src/client/Item/Items.ts
src/common/helper.ts
src/request/RequestHandler.ts
test/functional/client.spec.ts
The script failed with exit code 1

[southpolesteve]

Not good! @zfoster Can you look at this next week? Something not quite right in CI

[zfoster]

Yep - I already noticed this. We seem to have conflicting formatting and it might be that CI isn’t failing on either because I’ve noticed lint changes that change files back to the way they were before but nothing fails even locally. Should be able to fix.

[deyaaeldeen]

@southpolesteve and @zfoster, I am interested to know whether there is a conflict between prettier and the linter. Please open an issue with more details about that conflict if it exists.

[ramya-rao-a]

@deyaaeldeen Some context on the linting story for cosmos.

• tslint is used in the build script and therefore fails the build when there is any linting errors introduced.
• The lint script uses our eslint plugin. The reason this is not used in CI is because there are still linting errors reported by it. [Cosmos] Make eslint step mandatory and revert tslint #5431 is the issue tracking the work needed to fix these errors and the update the build script to use the lint script instead of the tslint tool.

Cosmos already runs prettier and verifies it in CI.

I don't see anything in any of the npm scripts in the package.json file that would suggest that prettier is run in CI. Is this something we missed when porting cosmos over to the mono repo? cc @HarshaNalluru

Yep - I already noticed this. We seem to have conflicting formatting and it might be that CI isn’t failing on either because I’ve noticed lint changes that change files back to the way they were before but nothing fails even locally. Should be able to fix.

@zfoster I don't see tslint reverting any of the changes introduced in this PR.

[ramya-rao-a]

@southpolesteve, @zfoster, Please provide your thoughts on #10219 as well.

[HarshaNalluru]

I don't see anything in any of the npm scripts in the package.json file that would suggest that prettier is run in CI. Is this something we missed when porting cosmos over to the mono repo?

As part of onboarding the cosmos SDK to the central repo, prettier config that was used by cosmos has been removed in favor of the prettier config from the central repo. Other than that, the build command was updated to not have check-format(prettier) and lint(tslint) steps.
#5429 added the lint(tslint) step back but check-format was never added to the build step.

With this PR, @deyaaeldeen has done the formatting changes according to the prettier config from the central repo, which is good.
In this same PR, we can make check-format command a part of the build step like before to avoid any regressions similar to the lint step.

We can remove the check-format and lint commands from the build command once these commands have mandatory tasks in our CI builds similar to the Check api extractor output changes/Analyze Readmes. That's a discussion for another day.
cc: @KarishmaGhiya

[ramya-rao-a]

In this same PR, we can make check-format command a part of the build step like before to avoid any regressions similar to the lint step.

I would recommend not to have a cosmos only formatting solution and instead have a repo wide solution for Track 2 packages

[ramya-rao-a]

@zfoster, @southpolesteve,

Given the above context and the upcoming work in #10219 (comment), we should be good to go with the changes in this PR. That ok?

[southpolesteve]

It does look like we missed prettier in the initial repo migration. Whoops!

@zfoster and I are both very pro-prettier and use the vscode extension which is why we haven't regressed too much. Merging this is fine with me. I don't have an opinion on the git hook as I haven't used them much.

The only thing I care about is that format, lint, or any other code quality check (api extractor..?) is enforced by CI.

[ramya-rao-a]

Thanks @southpolesteve

api-extractor check is already in place in CI
you know the status for lint
we will conclude on the format story and share the end result with you

@deyaaeldeen, Let's go ahead and merge the PR

[zfoster]

I'm good to go ahead with these changes as well