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

update 13-How-to-edit-wiki.md with first learnings from #142 #144

Open
mi-hol opened this issue Sep 4, 2023 · 11 comments
Open

update 13-How-to-edit-wiki.md with first learnings from #142 #144

mi-hol opened this issue Sep 4, 2023 · 11 comments

Comments

@mi-hol
Copy link
Contributor

mi-hol commented Sep 4, 2023

Current instructions for https://wiki.fome.tech/How-to-edit-wiki/#how-to-test-your-changes are incomplete/wrong
See #142 (comment) for details

@karniv00l
Copy link
Member

That's an overstatement, current instructions look fine

@mi-hol mi-hol changed the title update 13-How-to-edit-wiki.md with learnings from #142 update 13-How-to-edit-wiki.md with first learnings from #142 Sep 11, 2023
@mi-hol
Copy link
Contributor Author

mi-hol commented Sep 11, 2023

current instructions look fine

Did you consider the "version change" scenario when the comment was made?
If yes, how would you explain the resulting omissions encountered from following them in #142 ?

@karniv00l
Copy link
Member

I don't see correlation between broken links and different method of installing Node.js dependencies, and in your case, change installed in dependencies

Please note that current docs already mention when to "re-install" dependencies:

You'll need to run this command once when you first download the project, and again only if you package-lock.json file changes.

However, I was able to reproduce your no warning state, and it looks like a Docusaurus weird behavior (it shows warnings only on rebuilds) which is improved by #147

@karniv00l
Copy link
Member

karniv00l commented Sep 11, 2023

Note that this will only check markdown links while running dev server (npm start)

will always fail when file does not exist:

[ETB PID and Autotune](/07-Advanced-Features/ETB/ETB-PID.md) 

it won't validate regular links

will only fail during the build when link does not work:

[ETB PID and Autotune](/Advanced-Features/ETB/ETB-PID)

since they are not crawled before building (npm run build)

I can see that now we use both markdown links and regular links

@mi-hol
Copy link
Contributor Author

mi-hol commented Sep 11, 2023

I can see that now we use both markdown links and regular links

Sorry, not sure which name above refers to which type of link :(
Therefore I refer to the problematic link type in combination with "numbered prefix" approach as "bad" link syntax.
Also I'm not really understanding what is meant by the second comment.
I'm looking for a working solution for everybody not have all your skills (aka user friendly).

Did we not come across this minor but annoying difference previously? I seem to remember so.

And actually I was surprised that only the previously existing link "/Advanced-Features/ETB/ETB-PID.md" required the number prefix, while all others did NOT.
Would it make sense to change the custom FOME link checker to throw an error on "bad" link syntax? or is that already implemented via #147?

@mi-hol
Copy link
Contributor Author

mi-hol commented Sep 11, 2023

will only fail during the build when link does not work:

[ETB PID and Autotune](/Advanced-Features/ETB/ETB-PID)

is the preferred approach in combination with "numbered prefix", right?

@mi-hol
Copy link
Contributor Author

mi-hol commented Sep 11, 2023

used above link syntax in #148 & #149

@mi-hol
Copy link
Contributor Author

mi-hol commented Sep 11, 2023

Would it make sense to change the custom FOME link checker to throw an error on "bad" link syntax?

@karniv00l could you please comment on above question?
I'd be happy to change custom FOME link checker "scripts/linkValidator.js" to implement it if you agree.

@karniv00l
Copy link
Member

Sure, I did a little refactor to make it more clear

Regarding .md links, it's actually a big upside that they are automatically validated by the build script, however a downside is that you need to provide a full file path (with numbered prefix), which will cause a refactor anytime someone move the order position. I didn't know about that feature

@mi-hol
Copy link
Contributor Author

mi-hol commented Sep 12, 2023

Sure, I did a little refactor to make it more clear

@karniv00l The suggested approach (validateRelativeUrls(files);) would from my view not have the best performance due to multiple reads of all files.
I've chosen to add all checks in the file read loop. (see https://github.com/mi-hol/fome-wiki/blob/validateLinks/scripts/linkValidator.js#L20)
Its not working yet but are you ok with this changed approach?

@karniv00l
Copy link
Member

sure

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

No branches or pull requests

2 participants