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

vdoc: cleanup get_module_list, add unit tests #21057

Merged
merged 6 commits into from
Apr 18, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Mar 18, 2024

Cleans up the get_module_list function and adds simple unit tests for it and get_ignore_paths. Hoping this creates a better base for further development of such features. E.g. I'd like to add the functionality for a .vfmtignore - similar to a .prettierignore, but for now with the simple functionality of the .vdocignore - in a follow up.

@ttytm ttytm force-pushed the vdoc/simplify-ignore-add-test branch from b41ad9c to 71c0cf3 Compare March 18, 2024 17:09
@ttytm ttytm marked this pull request as draft March 18, 2024 17:45
@JalonSolov
Copy link
Contributor

Did you mean .vdocignore? Alex does not want .vfmtignore - he doesn't want ANY options to vfmt, as it is supposed to stay opinionated.

@ttytm
Copy link
Member Author

ttytm commented Mar 18, 2024

The commit is related to cleaning up tests the .vdocignore functionality.

A feature I would like to implement is a .vfmtignore. There already is // vfmt off which is the same functionality that a .vfmtignore would do.

@ttytm
Copy link
Member Author

ttytm commented Mar 18, 2024

I'm coming to the .vfmtignore as you currently can't run v fmt on the. v-analzyer project.
V is ignoring formatting for some internal files hardcoded. For other projects and for the V project itself, instead of hardcoding / adding vfmt off to every problematic file, a .vfmtignore would be just logical for me. There would be no new options.

But I'm not too attached to the feature, so if it's not wanted improving .vdocignore is also fine 👍

@spytheman
Copy link
Member

spytheman commented Mar 20, 2024

The commit is related to cleaning up tests the .vdocignore functionality.

A feature I would like to implement is a .vfmtignore. There already is // vfmt off which is the same functionality that a .vfmtignore would do.

No, it is not. The difference is scope.
A feature like .vfmtignore will shift the balance towards making it extremely easy to just put a * pattern in it. I want the balance to be towards most projects using vfmt, and thus towards consistent formatting across the entire V ecosystem, since that is the easiest.

To do that using // vfmt off, you would have to put that in every single .v file, since it was designed for local changes, not for global ones, like a .vfmtignore file would allow.

@spytheman
Copy link
Member

V is ignoring formatting for some internal files hardcoded. For other projects and for the V project itself, instead of hardcoding / adding vfmt off to every problematic file`

It is better for the ecosystem to gradually fix those, not to make it easier for projects to use a global .vfmtignore file.

@ttytm
Copy link
Member Author

ttytm commented Mar 20, 2024

The decision to not have a .vfmtignore is comprehensible. I'll won't submit a PR. The issue of not beeing able to easily run v fmt -verify/-c . on a project like v analyzer and tell if there are formatting errors next to internal errors was solved by a recent PR extending the exit codes.

@spytheman
Copy link
Member

v-analyzer can just use .vv for those files, similar to what V itself does.
For example, after:

rename s/.v$/.vv/gm metadata/stubs/*.v
rename s/.v$/.vv/gm editors/code/syntaxes/tests/*.v
rename s/.v$/.vv/gm tests/testdata/documentation/*.v
rename s/.v$/.vv/gm tests/testdata/types/*.v

vfmt does not complain anymore:

#0 11:58:29 ᛋ main /v/v-analyzer❱v fmt -c .
#0 11:59:28 ᛋ main /v/v-analyzer❱

@ttytm ttytm force-pushed the vdoc/simplify-ignore-add-test branch 2 times, most recently from 2ea7139 to d4d1ed6 Compare April 18, 2024 13:03
@ttytm ttytm marked this pull request as ready for review April 18, 2024 13:03
@ttytm ttytm force-pushed the vdoc/simplify-ignore-add-test branch 3 times, most recently from b193cf3 to ba29008 Compare April 18, 2024 13:17
@ttytm ttytm force-pushed the vdoc/simplify-ignore-add-test branch from ba29008 to d54384c Compare April 18, 2024 13:18
@ttytm ttytm force-pushed the vdoc/simplify-ignore-add-test branch from db65ffc to 5f34c61 Compare April 18, 2024 13:21
@ttytm ttytm force-pushed the vdoc/simplify-ignore-add-test branch from 079d72a to 373841e Compare April 18, 2024 14:20
@spytheman spytheman merged commit a6087d0 into vlang:master Apr 18, 2024
50 checks passed
@ttytm ttytm deleted the vdoc/simplify-ignore-add-test branch April 18, 2024 18:51
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.

3 participants