Skip to content

Fix #1879 Add .md extension to files created in sidebar if no extension is given #1886

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

Merged
merged 2 commits into from
Feb 8, 2020
Merged

Fix #1879 Add .md extension to files created in sidebar if no extension is given #1886

merged 2 commits into from
Feb 8, 2020

Conversation

davisriedel
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
New tests added? not needed
Fixed tickets Fixes #1879
License MIT

Description

Fixes Issue #1879

--

Please don't submit /dist files with your PR!

@@ -173,7 +173,14 @@ const actions = {

CREATE_FILE_DIRECTORY ({ commit, state }, name) {
const { dirname, type } = state.createCache

if (type === 'file') {
const CHECK_MD_EXTENSION = /.*\.(?:markdown|mdown|mkdn|md|mkd|mdwn|mdtxt|mdtext|text|txt)$/i
Copy link
Contributor

@alerque alerque Jan 29, 2020

Choose a reason for hiding this comment

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

What about people that use RMarkdown (.rmd) or use .pandoc as their file extension? Hard coding these seems like an ugly hack that will never cover all the bases. How about just checking if there is any extension at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree to you, but the problem is, that the sidebar does not allow other extensions in the first place. Have a look at src/common/filesystem/paths.js

export const MARKDOWN_EXTENSIONS = [
  'markdown',
  'mdown',
  'mkdn',
  'md',
  'mkd',
  'mdwn',
  'mdtxt',
  'mdtext',
  'text',
  'txt'
]

// ...

/**
 * Returns true if the filename matches one of the markdown extensions.
 *
 * @param {string} filename Path or filename
 */
export const hasMarkdownExtension = filename => {
  if (!filename || typeof filename !== 'string') return false
  return MARKDOWN_EXTENSIONS.some(ext => filename.toLowerCase().endsWith(`.${ext}`))
}

If you create for example a .pandoc file in the sidebar, it is created but Mark Text does not allow to open it and greys it out. That's the reason why I decided to check for these extensions as well.
Maybe we could add the default .md extension if no extension is given at all, and warn the user if he creates a file with a disallowed extension?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I guess it would be acceptable for now to match what the sidebar already does, but both the sidebar and this should probably be expanded to cover more options in the future. Would it be possible to use a single constant for this so it doesn't need to be updated in two places and create possible miss-mataches?

Copy link
Contributor

@fxha fxha left a comment

Choose a reason for hiding this comment

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

Thanks @dxxl for your contribution. I agree with @alerque to use the hasMarkdownExtension function from common/filesystem/paths.js (import { hasMarkdownExtension } from 'common/filesystem/paths') to simplify changes in the future. Otherwise LGTM 👍

@davisriedel
Copy link
Contributor Author

Thanks for the Feedback! There are several places where either the regex I used is used or markdown extensions are matched another way. Maybe we should consider to use the hasMarkdownExtension function in all those places?

…hecked.

Allowed markdown extensions are handeled in several places in different ways. Use the hasMarkdownFunction from common/filesystem/paths.js to allow to easily update the allowed extensions when needed.
@Jocs
Copy link
Member

Jocs commented Feb 6, 2020

Thanks for the Feedback! There are several places where either the regex I used is used or markdown extensions are matched another way. Maybe we should consider to use the hasMarkdownExtension function in all those places?

Good idea, will you change them in this PR?

@Jocs Jocs merged commit 8623243 into marktext:develop Feb 8, 2020
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.

New file function dont add extension automaticaly
5 participants