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

fix: homogenize markdown across files using markdownlint rules #6988

Merged

Conversation

davorpa
Copy link
Member

@davorpa davorpa commented Aug 6, 2022

What does this PR do?

Improve repo

For resources

Description

Why is this valuable (or not)?

Homogenizes using markdownlint rules (see also #6829 to check which are applied)

See each commit title/description to understand better what we are doing

Checklist:

  • Read our contributing guidelines.
  • Search for duplicates.
  • Include author(s) and platform where appropriate.
  • Put lists in alphabetical order, correct spacing.
  • Add needed indications (PDF, access notes, under construction).
  • Used an informative name for this pull request.

Follow-up

  • Check the status of GitHub Actions and resolve any reported warnings!

@davorpa davorpa self-assigned this Aug 6, 2022
@davorpa davorpa added the conflicts Conflict(s) need to be resolved label Aug 6, 2022
@davorpa
Copy link
Member Author

davorpa commented Aug 6, 2022

Conflicts are with merges of

…genize to solve conflicts

Cherry picked until 44579d3 commit
@davorpa davorpa added conflicts Conflict(s) need to be resolved and removed conflicts Conflict(s) need to be resolved labels Aug 6, 2022
@davorpa
Copy link
Member Author

davorpa commented Aug 6, 2022

Conflicts remains with

merges due to final blank lines

…to solve conflicts

Cherry picket until 04cf8c8 commit
@davorpa davorpa removed the conflicts Conflict(s) need to be resolved label Aug 6, 2022
@davorpa davorpa marked this pull request as ready for review August 6, 2022 07:42
@LuigiImVector
Copy link
Member

Wasn't it enough to change the value of "indent" to 2? Even if that meant disabling MD005.

@LuigiImVector LuigiImVector added 🗣️ locale:en Resources addressing "English" language 🗣️ locale:es Resources addressing "Spanish / español" language 🗣️ locale:fr Resources addressing "French / français" language 🗣️ locale:de Resources addressing "German / Deutsch" language 🗣️ locale:it Resources addressing "Italian / italiano" language 🗣️ locale:pt Resources addressing "Portuguese / Brazilian" language 🗣️ locale:ru Resources addressing "Russian / Русский язык" language 🗣️ locale:zh Resources addressing "Chinese" language labels Aug 6, 2022
@davorpa
Copy link
Member Author

davorpa commented Aug 6, 2022

Wasn't it enough to change the value of "indent" to 2? Even if that meant disabling MD005.

According to .editorconfig file, indentation is using 4 spaces

end_of_line = lf
indent_size = 4
indent_style = space
insert_final_newline = true

Is this why enable MD005/MD007 to use that number of spaces.

// MD005/list-indent - Inconsistent indentation for list items at the same level
"MD005": true,
// MD006/ul-start-left - Consider starting bulleted lists at the beginning of the line
"MD006": true,
// MD007/ul-indent - Unordered list indentation
"MD007": {
// Spaces for indent (sync with .editorconfig)
"indent": 4,
// Whether to indent the first level of the list
"start_indented": false
},
// MD009/no-trailing-spaces - Trailing spaces
"MD009": {
// Spaces for line break (sync with .editorconfig)
"br_spaces": 4,
// Allow spaces for empty lines in list items
"list_item_empty_lines": false,
// Include unnecessary breaks
"strict": false
},
// MD010/no-hard-tabs - Hard tabs
"MD010": {
// Include code blocks
"code_blocks": true,
// Number of spaces for each hard tab
"spaces_per_tab": 4
},

Let's discuss about it if is wrong

@LuigiImVector
Copy link
Member

On the official kramdown website it says:

kramdown assumes that tab stops are set at multiples of four. This is especially important when using tabs for indentation in lists. Also, tabs may only be used at the beginning of a line when indenting text and must not be preceded by spaces. Otherwise the results may be unexpected.

So I think 4 indent_size it's okay.

@davorpa davorpa added the conflicts Conflict(s) need to be resolved label Aug 8, 2022
@davorpa
Copy link
Member Author

davorpa commented Aug 8, 2022

Conflicts again due to #6989 merge which modifies TOC entries adding DL, ML, NLP...

@davorpa davorpa added conflicts Conflict(s) need to be resolved and removed conflicts Conflict(s) need to be resolved labels Aug 8, 2022
@davorpa
Copy link
Member Author

davorpa commented Aug 9, 2022

More conflicts after merge #6986 which modify TOC entries removing impress.js and has other indent problems

@davorpa davorpa removed the conflicts Conflict(s) need to be resolved label Aug 9, 2022
Copy link
Collaborator

@eshellman eshellman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation behind replacing translated words with Englishe words?

@davorpa
Copy link
Member Author

davorpa commented Aug 10, 2022

What's the motivation behind replacing translated words with Englishe words?

I suppose you're referring to 4d6a74e commit which normalizes TOC section header to Index word, it doesn't?

I don't know yet how it works but thinking in the fpb-parser, I hope it helps to filter this section from processing. It'll be easy to do using same word and heading level (3 #) or even more... develop a fpb-lint rule expecting this Index branch as markdown listing element

https://github.com/EbookFoundation/free-programming-books-parser/blob/5eaf00bfd861856a85b2ba6f2d2aa282fab55a69/index.js#L178-L192

As we can see... Index is expected according to current sourcecode. I open EbookFoundation/free-programming-books-parser#1 to address it

Of course, make a git revert is always a good option here if it isn't relevant or already contemplated

@eshellman
Copy link
Collaborator

What's the motivation behind replacing translated words with Englishe words?

I suppose you're referring to 4d6a74e commit which normalizes TOC section header to Index word, it doesn't?

I don't know yet how it works but thinking in the fpb-parser, I hope it helps to filter this section from processing. It'll be easy to do using same word and heading level (3 #) or even more... develop a fpb-lint rule expecting this Index branch as markdown listing element

https://github.com/EbookFoundation/free-programming-books-parser/blob/5eaf00bfd861856a85b2ba6f2d2aa282fab55a69/index.js#L178-L192

As we can see... Index is expected according to current sourcecode. I open EbookFoundation/free-programming-books-parser#1 to address it

Of course, make a git revert is always a good option here if it isn't relevant or already contemplated

best not to fix something unless its broken

@davorpa
Copy link
Member Author

davorpa commented Aug 11, 2022

best not to fix something unless its broken

Then reverting 4d6a74e 😄

@eshellman
Copy link
Collaborator

LGTM. Ready to merge?

@davorpa
Copy link
Member Author

davorpa commented Aug 12, 2022

LGTM. Ready to merge?

Yes, sure! No more work on my own.

Can I merge it to get some GitHub profile achievement badges?

@eshellman
Copy link
Collaborator

eshellman commented Aug 12, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗣️ locale:ar Resources addressing "Arabic / العربية" language 🗣️ locale:bn Resources addressing "Bengali / বাংলা" language 🗣️ locale:cs Resources addressing "Czech / čeština / český jazyk" language 🗣️ locale:de Resources addressing "German / Deutsch" language 🗣️ locale:el Resources addressing "Greek / Hellenic / ελληνικά" language 🗣️ locale:en Resources addressing "English" language 🗣️ locale:es Resources addressing "Spanish / español" language 🗣️ locale:et Resources addressing "Estonian / eesti keel" language 🗣️ locale:fa_IR Resources addressing "Persian / Farsi (Iran) / فارسى" language 🗣️ locale:fr Resources addressing "French / français" language 🗣️ locale:he Resources addressing "Hebrew / עברית" language 🗣️ locale:hi Resources addressing "Hindi / हिन्दी" language 🗣️ locale:hu Resources addressing "Hungarian / magyar / magyar nyelv" language 🗣️ locale:id Resources addressing "Indonesian" language 🗣️ locale:it Resources addressing "Italian / italiano" language 🗣️ locale:kk Resources addressing "Kazakh / қазақша" language 🗣️ locale:km Resources addressing "Khmer / Cambodian / ខ្មែរ" language 🗣️ locale:ko Resources addressing "Korean / 한국어 [韓國語]" language 🗣️ locale:ml Resources addressing "Malayalam / മലയാളം" language 🗣️ locale:nl Resources addressing "Dutch / Nederlands" language 🗣️ locale:pl Resources addressing "Polish / polski" language 🗣️ locale:pt Resources addressing "Portuguese / Brazilian" language 🗣️ locale:ro Resources addressing "Romanian (Romania) / limba română / român" language 🗣️ locale:ru Resources addressing "Russian / Русский язык" language 🗣️ locale:si Resources addressing "Sinhala / සිංහල" language 🗣️ locale:sk Resources addressing "Slovak / slovenčina" language 🗣️ locale:th Resources addressing "Thai / ไทย" language 🗣️ locale:tr Resources addressing "Turkish / Türkçe" language 🗣️ locale:uk Resources addressing "Ukrainian / Українська" language 🗣️ locale:vi Resources addressing "Vietnamese / Tiếng Việt" language 🗣️ locale:zh Resources addressing "Chinese" language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants