[tools] Fix API doc string validation #2344

sarfarazsiddiquii · 2024-04-19T22:40:07Z

This PR solves the issue (ref #2331).

The warnings from stdlib/src/ have been resolved and integrated into CI.

JoeLoser · 2024-04-19T23:36:08Z

Hey @sarfarazsiddiquii, thanks for the contribution! Do you mind running mojo format stdlib/src/collections/optional.mojo to fix the CI error here? I think it would also be great to mention the API doc string validation in the contributing or style guide, maybe somewhere after this section.

JoeLoser · 2024-04-20T00:16:35Z

I also think it would be nice to run the API doc string validation in

mojo/stdlib/scripts/build-stdlib.sh

Line 40 in 20737dc

mojo package "${STDLIB_PATH}" -o "${FULL_STDLIB_PACKAGE_PATH}"

for example after we build the stdlib.mojopkg (and before we run the tests). What do you think?

I think we should incorporate this mojo doc validate command in our scripts in some shape or form so developers are notified of any issues before CI fails. WDYT @ConnorGray?

stdlib/src/builtin/reversed.mojo

sarfarazsiddiquii · 2024-04-20T07:33:09Z

@JoeLoser Thanks for the feedback, according to your request i have made changes in the codebase and the documentation as well.

sarfarazsiddiquii · 2024-04-20T07:44:16Z

@JoeLoser i am still getting errors ( here -
Standard Library tests and examples / test-examples) after new changes, can you help me?

stdlib/scripts/build-stdlib.sh

gabrieldemarmiesse · 2024-04-21T09:51:56Z

stdlib/scripts/run-tests.sh

@@ -23,6 +23,12 @@ mkdir -p "${BUILD_DIR}"

 source "${SCRIPT_DIR}"/build-stdlib.sh

+echo "Validating API documentation strings in the Standard Library."


Same here, testing the docstrings is more something to do when a PR is nearly done. Something like a pre-commit.
Contributors might want to run tests without checking the docstrings first.

I agree this is more akin to mojo format sort of thing - something needed to be fixed up prior to landing a PR. After this lands, do you have any interest in exploring hooking this up with the pre-commit things you've set up recently? I think that would be great.

gabrieldemarmiesse · 2024-04-21T09:53:00Z

stdlib/docs/style-guide.md

@@ -59,6 +59,15 @@ pre-commit install
 ```
 and that's it!

+ #### API Doc String Validation
+
+Mojo provides a command line utility, `mojo doc validate`, to validate the API doc strings in your code. This ensures that your doc strings are correctly formatted and consistent with the Mojo style guidelines.


Suggested change

Mojo provides a command line utility, `mojo doc validate`, to validate the API doc strings in your code. This ensures that your doc strings are correctly formatted and consistent with the Mojo style guidelines.

Mojo provides a command line utility, `mojo doc`, to validate the API doc strings in your code. This ensures that your doc strings are correctly formatted and consistent with the Mojo style guidelines.

gabrieldemarmiesse · 2024-04-21T09:53:38Z

stdlib/docs/style-guide.md

+Mojo provides a command line utility, `mojo doc validate`, to validate the API doc strings in your code. This ensures that your doc strings are correctly formatted and consistent with the Mojo style guidelines.
+
+```bash
+> mojo doc validate example.mojo


Suggested change

> mojo doc validate example.mojo

> mojo doc -warn-missing-doc-strings -o /dev/null example.mojo

mojo doc validate doesn't exist. It's mojo doc

gabrieldemarmiesse · 2024-04-21T09:57:50Z

stdlib/scripts/build-stdlib.sh

@@ -39,4 +39,11 @@ STDLIB_PACKAGE_NAME="stdlib.mojopkg"
 FULL_STDLIB_PACKAGE_PATH="${BUILD_DIR}"/"${STDLIB_PACKAGE_NAME}"
 mojo package "${STDLIB_PATH}" -o "${FULL_STDLIB_PACKAGE_PATH}"

+echo "Validating API documentation strings in the Standard Library."
+
+mojo doc validate "${STDLIB_PATH}" || {


mojo doc validate doesn't exist, it's mojo doc. This is what causes the CI to fail.

gabrieldemarmiesse · 2024-04-21T09:59:40Z

stdlib/scripts/run-tests.sh

@@ -23,6 +23,12 @@ mkdir -p "${BUILD_DIR}"

 source "${SCRIPT_DIR}"/build-stdlib.sh

+echo "Validating API documentation strings in the Standard Library."
+mojo doc validate "${REPO_ROOT}/stdlib/src" || {


Same here, mojo doc validate doesn't exist. It's mojo doc, this is why the CI Doesn't understand what is going on (but I think the error message is not correct, mojo doc should output that the directory validate doesn't exist.)

sarfarazsiddiquii · 2024-04-21T10:52:11Z

Thank you, @gabrieldemarmiesse, for pointing out the issues in my previous commits. I have resolved all the errors as suggested, and everything seems to be working as expected. Please let me know if there are any other changes required in this PR.

gabrieldemarmiesse · 2024-04-21T11:42:05Z

Looks good! The only thing missing is the DCO. Take a look at the instructions here: https://github.com/modularml/mojo/pull/2344/checks?check_run_id=24070604633

sarfarazsiddiquii · 2024-04-21T13:03:37Z

Looks good! The only thing missing is the DCO. Take a look at the instructions here: https://github.com/modularml/mojo/pull/2344/checks?check_run_id=24070604633

Done.

JoeLoser · 2024-04-21T15:41:07Z

.github/workflows/standard_library_tests_and_examples.yml

+      - name: Validate documentation strings
+        run: |
+          mojo doc -warn-missing-doc-strings -o /dev/null stdlib/src/
+          if [ $? -ne 0 ]; then


Suggestion Unfortunately, this won't work as you hope since the above command still exits with exit code 0 since it produces warnings (not errors). You can verify this locally:

mojo doc -warn-missing-doc-strings -o /dev/null stdlib/src/ Included from /Users/joe/dev/mojo/stdlib/src/collections/__init__.mojo:1: /Users/joe/dev/mojo/stdlib/src/collections/dict.mojo:49:7: warning: public symbol 'StringableKeyElement' is missing a doc string trait StringableKeyElement(KeyElement, Stringable): ^ Included from /Users/joe/dev/mojo/stdlib/src/os/__init__.mojo:1: /Users/joe/dev/mojo/stdlib/src/os/os.mojo:274:4: warning: function takes parameters, but no 'Parameters' in doc string fn remove[pathlike: os.PathLike](path: pathlike) raises: ^ Included from /Users/joe/dev/mojo/stdlib/src/os/__init__.mojo:1: /Users/joe/dev/mojo/stdlib/src/os/os.mojo:298:4: warning: function takes parameters, but no 'Parameters' in doc string fn unlink[pathlike: os.PathLike](path: pathlike) raises: ^ Included from /Users/joe/dev/mojo/stdlib/src/testing/__init__.mojo:1: /Users/joe/dev/mojo/stdlib/src/testing/testing.mojo:115:4: warning: function takes parameters, but no 'Parameters' in doc string fn assert_equal[T: Testable](lhs: T, rhs: T, msg: String = "") raises: ^ Included from /Users/joe/dev/mojo/stdlib/src/testing/__init__.mojo:1: /Users/joe/dev/mojo/stdlib/src/testing/testing.mojo:175:4: warning: function takes parameters, but no 'Parameters' in doc string fn assert_not_equal[T: Testable](lhs: T, rhs: T, msg: String = "") raises:

combined with echo $? which reports the exit code of 0.

To "fix" this problem, you can use the check-file-is-empty.py script from #2118 that got removed (just bring it back for this PR).

Longer term, I think we should file a GitHub issue and have a discussion with @River707 and @modocache on their take on this command. The current structure of producing warnings for invalid API doc strings and exiting with an exit code of 0, in my opinion, pushes complexity to every user (such as with this check-file-is-empty.py script). I'd rather have some option (even if it's -error-missing-doc-strings or name to be bikeshedded) which would give me a non-zero exit code if there are any invalid doc strings. I think that will be the common use case for users of this command.

That is correct, i will look into it and make changes accordingly by looking at the mentioned file.

JoeLoser · 2024-04-21T15:42:34Z

stdlib/scripts/run-tests.sh

@@ -23,6 +23,12 @@ mkdir -p "${BUILD_DIR}"

 source "${SCRIPT_DIR}"/build-stdlib.sh

+echo "Validating API documentation strings in the Standard Library."


I agree this is more akin to mojo format sort of thing - something needed to be fixed up prior to landing a PR. After this lands, do you have any interest in exploring hooking this up with the pre-commit things you've set up recently? I think that would be great.

sarfarazsiddiquii · 2024-04-21T19:58:13Z

@JoeLoser In the commit, I restored the code in the file check-file-is-empty.py, and integrated it into Standard Library tests and examples which will check if the file warnings.txt contains any warnings. If the file is not empty, meaning it has warnings, the test will fail.

This commit caused license issue can you help me with it, and also review if the changes i made are correct?
Thank you.

Signed-off-by: sarfaraz siddiqui <sarfarazsiddiqui199@gmail.com>

JoeLoser · 2024-04-27T13:51:04Z

@JoeLoser In the commit, I restored the code in the file check-file-is-empty.py, and integrated it into Standard Library tests and examples which will check if the file warnings.txt contains any warnings. If the file is not empty, meaning it has warnings, the test will fail.

This commit caused license issue can you help me with it, and also review if the changes i made are correct? Thank you.

Hey! Sorry for the delay in response — I was on PTO earlier this week. The CI failure here is trying to tell you that we need to add the license part at the top of the file of stdlib/scripts/check-file-is-empty.py. However, you already have the license, so I suspect this is an issue with the license file checker itself. Specifically, this line assumes the first line will be the beginning of the license itself. However, in your case, it's a shebang, meaning the check won't pass, pedantically. To fix this, one approach would be to do a little bit of parsing (see if it starts with ! and if so, skip that line (when you hit a newline character)) and assume the rest of the file is license + rest. Then you can keep the same startswith logic for the license, etc. Thoughts, @gabrieldemarmiesse?

gabrieldemarmiesse · 2024-04-27T14:30:35Z

Sure I can do that. In the meantime, you can remove the shebang and just call the script with python3 path_to_script.py that should do the trick

sarfarazsiddiquii · 2024-04-28T15:34:35Z

@JoeLoser I did what you suggested however the problem is not yet resolved.
Can you help me with it.

gabrieldemarmiesse · 2024-04-28T17:47:02Z

Hi, I took at look at the pull request and saw that if we incorporated it in the pre-commit hook, it should make things much simpler. I pulled your commits and modified the code a bit more and arrived at this result: nightly...gabrieldemarmiesse:mojo:include_to_pre_commit

@sarfarazsiddiquii Do you think I should open a new PR with my branch? Or do you want to pull my commits to your branch maybe? It's as you wish. If I make a new PR with my branch, since your commits are there, you'll still be credited as one of the authors.

sarfarazsiddiquii · 2024-04-29T15:28:28Z

Hi, I took at look at the pull request and saw that if we incorporated it in the pre-commit hook, it should make things much simpler. I pulled your commits and modified the code a bit more and arrived at this result: nightly...gabrieldemarmiesse:mojo:include_to_pre_commit

@sarfarazsiddiquii Do you think I should open a new PR with my branch? Or do you want to pull my commits to your branch maybe? It's as you wish. If I make a new PR with my branch, since your commits are there, you'll still be credited as one of the authors.

Thanks @gabrieldemarmiesse for helping me on this issue. I think you should open a new PR since it has both our contributions.

gabrieldemarmiesse · 2024-04-29T15:44:07Z

Great :) #2444 was opened I think you can close this PR

sarfarazsiddiquii requested a review from a team as a code owner April 19, 2024 22:40

JoeLoser changed the title ~~Fix: API doc string validation~~ [tools] Fix API doc string validation Apr 19, 2024

JoeLoser requested a review from modocache April 19, 2024 23:37

ConnorGray reviewed Apr 20, 2024

View reviewed changes

stdlib/src/builtin/reversed.mojo Outdated Show resolved Hide resolved

sarfarazsiddiquii requested review from scottamain and arthurevans as code owners April 20, 2024 07:29

gabrieldemarmiesse reviewed Apr 21, 2024

View reviewed changes

sarfarazsiddiquii force-pushed the newbranch branch from 292f72d to c72738a Compare April 21, 2024 12:59

JoeLoser reviewed Apr 21, 2024

View reviewed changes

sarfarazsiddiquii force-pushed the newbranch branch from df44cc8 to 5a2fd67 Compare April 21, 2024 19:51

sarfarazsiddiquii added 5 commits April 22, 2024 01:29

API doc string validation

03e22be

Signed-off-by: sarfaraz siddiqui <sarfarazsiddiqui199@gmail.com>

Enhancement in code

c8df61e

Signed-off-by: sarfaraz siddiqui <sarfarazsiddiqui199@gmail.com>

error in ren-tests.sh

1a4cfac

Signed-off-by: sarfaraz siddiqui <sarfarazsiddiqui199@gmail.com>

resolving mojo doc

3f55494

Signed-off-by: sarfaraz siddiqui <sarfarazsiddiqui199@gmail.com>

added a script to check for warnings

e614d0c

Signed-off-by: sarfaraz siddiqui <sarfarazsiddiqui199@gmail.com>

sarfarazsiddiquii force-pushed the newbranch branch from 5a2fd67 to e614d0c Compare April 21, 2024 19:59

sarfarazsiddiquii requested review from JoeLoser and gabrieldemarmiesse April 22, 2024 18:36

JoeLoser mentioned this pull request Apr 27, 2024

[mojo-stdlib] Create InlineArray type #2294

Closed

JoeLoser mentioned this pull request Apr 27, 2024

[stdlib] Add repr() function and Representable trait #2361

Closed

Fixed check-licences.mojo

411fd9a

sarfarazsiddiquii requested a review from a team as a code owner April 28, 2024 15:24

Resolved merge conflicts

46cd39f

sarfarazsiddiquii closed this Apr 29, 2024

JoeLoser mentioned this pull request Apr 29, 2024

[tools] Add a docstring check to the pre-commit #2444

Closed

gabrieldemarmiesse mentioned this pull request Apr 29, 2024

[Feature Request] Provide an option to make mojo doc fail if any warnings are triggered #2447

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tools] Fix API doc string validation #2344

[tools] Fix API doc string validation #2344

sarfarazsiddiquii commented Apr 19, 2024

JoeLoser commented Apr 19, 2024

JoeLoser commented Apr 20, 2024

sarfarazsiddiquii commented Apr 20, 2024

sarfarazsiddiquii commented Apr 20, 2024 •

edited

gabrieldemarmiesse Apr 21, 2024

JoeLoser Apr 21, 2024

gabrieldemarmiesse Apr 21, 2024

gabrieldemarmiesse Apr 21, 2024

gabrieldemarmiesse Apr 21, 2024

gabrieldemarmiesse Apr 21, 2024

sarfarazsiddiquii commented Apr 21, 2024

gabrieldemarmiesse commented Apr 21, 2024

sarfarazsiddiquii commented Apr 21, 2024

JoeLoser Apr 21, 2024

sarfarazsiddiquii Apr 21, 2024 •

edited

JoeLoser Apr 21, 2024

sarfarazsiddiquii commented Apr 21, 2024 •

edited

JoeLoser commented Apr 27, 2024

gabrieldemarmiesse commented Apr 27, 2024

sarfarazsiddiquii commented Apr 28, 2024

gabrieldemarmiesse commented Apr 28, 2024

sarfarazsiddiquii commented Apr 29, 2024

gabrieldemarmiesse commented Apr 29, 2024

		@@ -23,6 +23,12 @@ mkdir -p "${BUILD_DIR}"

		source "${SCRIPT_DIR}"/build-stdlib.sh

		echo "Validating API documentation strings in the Standard Library."

	Mojo provides a command line utility, `mojo doc validate`, to validate the API doc strings in your code. This ensures that your doc strings are correctly formatted and consistent with the Mojo style guidelines.
	Mojo provides a command line utility, `mojo doc`, to validate the API doc strings in your code. This ensures that your doc strings are correctly formatted and consistent with the Mojo style guidelines.

	> mojo doc validate example.mojo
	> mojo doc -warn-missing-doc-strings -o /dev/null example.mojo

[tools] Fix API doc string validation #2344

[tools] Fix API doc string validation #2344

Conversation

sarfarazsiddiquii commented Apr 19, 2024

JoeLoser commented Apr 19, 2024

JoeLoser commented Apr 20, 2024

sarfarazsiddiquii commented Apr 20, 2024

sarfarazsiddiquii commented Apr 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarfarazsiddiquii commented Apr 21, 2024

gabrieldemarmiesse commented Apr 21, 2024

sarfarazsiddiquii commented Apr 21, 2024

Choose a reason for hiding this comment

sarfarazsiddiquii Apr 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarfarazsiddiquii commented Apr 21, 2024 • edited

JoeLoser commented Apr 27, 2024

gabrieldemarmiesse commented Apr 27, 2024

sarfarazsiddiquii commented Apr 28, 2024

gabrieldemarmiesse commented Apr 28, 2024

sarfarazsiddiquii commented Apr 29, 2024

gabrieldemarmiesse commented Apr 29, 2024

sarfarazsiddiquii commented Apr 20, 2024 •

edited

sarfarazsiddiquii Apr 21, 2024 •

edited

sarfarazsiddiquii commented Apr 21, 2024 •

edited