-
Notifications
You must be signed in to change notification settings - Fork 341
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
chore(docs): reorganize docs folder, add 404 checker #2125
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: moul <94029+moul@users.noreply.github.com>
# Conflicts: # docs/02-getting-started/03-local-setup/browsing-gno-source-code.md # docs/02-getting-started/03-local-setup/browsing-gnoland.md # docs/02-getting-started/03-local-setup/installation.md # docs/02-getting-started/03-local-setup/interacting-with-gnoland.md # docs/04-concepts/stdlibs/events.md
docs/Makefile
Outdated
|
||
# Build the linter | ||
build: | ||
cd linter && go build -o ./build/linter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool that you added a linter. However, please move it to misc/docs-linter
.
Keep this Makefile here.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to run it from the CI too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the linter: e7546dd
Adding the linter to the CI seems like it is out of scope for this PR - let's leave it for another one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the linter to the CI: 8fad8c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
There is a question on the hierarchy that we need to address later, but this system looks good for both humans and machines, whether on GitHub or from a Go script that would want to generate static pages.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2125 +/- ##
==========================================
- Coverage 54.64% 54.62% -0.02%
==========================================
Files 578 578
Lines 77877 77877
==========================================
- Hits 42554 42544 -10
- Misses 32150 32160 +10
Partials 3173 3173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to resolve some things before we move forward, please address the comments
|
||
require ( | ||
github.com/davecgh/go-spew v1.1.1 // indirect | ||
github.com/peterbourgon/ff/v3 v3.4.0 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to use this package for a single flag?
We can use the standard go flag library for the lint commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using our commands package (github.com/gnolang/gno/tm2/pkg/commands
) and I think ff/v3
is a dependency on it. I would keep it in for consistency throughout the codebase and the possibility that we add more flags later, seems like there is no harm in this.
misc/docs-linter/main.go
Outdated
) | ||
|
||
var ( | ||
ErrEmptyPath = errors.New("you need to pass in a path to scan") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commands.Metadata{ | ||
Name: "docs-linter", | ||
ShortUsage: "docs-linter [flags]", | ||
ShortHelp: "Finds broken 404 links in the .md files in the given folder & subfolders", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this lint, not just find broken links?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, this "linter" is supposed to only check for 404s, as stated in the original PR. Do you think docs-404-checker
would be a more appropriate name?
ShortHelp: "Finds broken 404 links in the .md files in the given folder & subfolders", | ||
}, | ||
cfg, | ||
func(ctx context.Context, args []string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's funny that the go linter was not run against this code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran a linter and fixed the unused variables: fe69c6a
misc/docs-linter/main.go
Outdated
return err | ||
} | ||
|
||
fmt.Printf("Linting %s...\n", abs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use commands.IO
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that fmt
is quite more simple and more known, and is a standard library, which is why I opted for it.
} | ||
} | ||
|
||
func removeDir(t *testing.T, dirPath string) func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not directly call this in the t.Cleanup
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the same functionality twice, so I've extracted it to a separate function. I think it makes the code more readable, and this is how I used to implement in in the past based on feedback from the team.
misc/docs-linter/util.go
Outdated
@@ -0,0 +1,107 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These funcs should be in the same file as where they're called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved all functions to the main.go
file in this commit: 3172ee8
misc/docs-linter/util.go
Outdated
// Lock the mutex before appending to results | ||
lock.Lock() | ||
*results = append(*results, fmt.Sprintf("%s (found in file: %s)", url, filePath)) | ||
lock.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can put this in a defer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved the locks out of this function here: 3172ee8
misc/docs-linter/util.go
Outdated
} | ||
|
||
// checkUrl checks if a URL is a 404 | ||
func checkUrl(lock *sync.Mutex, url string, filePath string, results *[]string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rewrite this function so it doesn't modify the inputs 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've purified the function to return an error based on the result, and then modify the array elsewhere: 3172ee8
(#2125)
) | ||
} | ||
|
||
func execLint(cfg *cfg, ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is supposed to be a linting suite for the documentation, then the main run loop shouldn't need to run a single linter, but be able to run multiple ones
Please create some kind of pipeline system 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea with this was to only have a link 404 checker. Reference this comment
I will resolve the merge conflicts after we merge this for consistency |
I've also added a CI workflow but I'm not fully sure I'm doing it right - any inputs would be appreciated. |
Staging is returning a 404: @albttx can you check this out? |
Description
Continuing #1999
Renames the docs with numbers, adds a Go linter and makefile to run it to check for 404 or broken links.
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description