Fix: New File Always Defaults to .md Instead of Selected Format (e.g., JSON) #5711
Conversation
|
Thank you for opening your first PR! 🎉 We would like to thank you already for your contribution. If everything checks out, we'll make sure to review the PR as soon as possible and give feedback. Please note a few things:
We are glad for every improvement that makes its way into the app, so we look forward to collaborating on merging this PR. |
|
Thank you for opening this! I have two things I would like to clarify before doing a code review:
In general, it helps reviewing PRs if you only change what is actually necessary. Currently it's a bit hard to disentangle what you have changed code-wise due to the various other changes you introduced. But again, thank you so far, I'm sure we can turn this into a great PR in no time! |
There was a problem hiding this comment.
Sorry for taking so long with the review. I believe this PR is going in the right direction. Most of my comments are relatively simple. There are a few things I'm unsure about and would like you to clarify some of the changes you did — I am not too familiar with the exact logic in this command because I haven't looked at it in quite some time, so I'm happy to listen to your justifications!
Finally, there are now a few linter/test errors that would need to be fixed (see the check annotations).
| @@ -72,24 +70,47 @@ export default class FileNew extends ZettlrCommand { | |||
| dirpath = arg.path | |||
| isFallbackDir = false | |||
| } else if (openDirectory !== null && await this._app.fsal.isDir(openDirectory)) { | |||
| // Choose the openDirectory | |||
| // Explicitly provided directory | |||
There was a problem hiding this comment.
Please undo this comment change
| ) { | ||
| // The user wishes/needs to confirm the filename and/or the directory | ||
| const chosenPath = await this._app.windows.saveFile(path.join(dirpath, generatedName)) | ||
| let defaultFileName = generatedName |
There was a problem hiding this comment.
Can we not reuse the generatedName variable here?
| } | ||
| break | ||
| case 'yaml': | ||
| if (!defaultFileName.endsWith('.yaml')) { |
There was a problem hiding this comment.
I would add a check if the name ends with .yml which is also a valid extension for YAML files. We can still default to .yaml below, that's absolutely fine.
| // There's no valid file extension given. We have to add one. By default | ||
| // we assume Markdown, but let ourselves be guided by the given type. | 10000||
| // Check if the file already has an extension, and don't add the default extension if it does | ||
| if (!path.extname(filename)) { |
There was a problem hiding this comment.
Please undo the changes in the if/switch construction here.
First, if(!path.extname(filename)) does not pass the linter, you'd need to explicitly compare it to a string w/o implicit boolean conversion. Second, this construction should've already worked. Or is there something problematic with how it works right now? Did I overlook something?
| if (await this._app.fsal.pathExists(absPath)) { | ||
| // Ask before overwriting | ||
| if (!await this._app.windows.shouldOverwriteFile(filename)) { |
| // First create the file | ||
| await this._app.fsal.writeTextFile(absPath, '') | ||
|
|
||
| // And directly thereafter, open the file |
There was a problem hiding this comment.
Please keep this comment. (In general, please do not delete comments unless they become invalid due to any change)
| pattern = uuid() | ||
| } | ||
|
|
||
| ext = ext || 'md' |
There was a problem hiding this comment.
ext is not optional, so this case is unnecessary. You can remove this.
| * @return {string} The new filename. | ||
| */ | ||
| export default function generateFilename (filenamePattern: string, idGenPattern: string): string { | ||
| export default function generateFilename (filenamePattern: string, idGenPattern: string, ext: string ): string { |
There was a problem hiding this comment.
Suggestion to strongly type the extension:
| export default function generateFilename (filenamePattern: string, idGenPattern: string, ext: string ): string { | |
| export default function generateFilename (filenamePattern: string, idGenPattern: string, ext: 'md'|'json'|'yaml'|'tex' ): string { |
| } | ||
|
|
||
| ext = ext || 'md' | ||
| pattern = pattern.replace(/%ext/g, ext) |
There was a problem hiding this comment.
You never introduced this %ext variable, so this change won't do much. I suggest to use instead uuid() + '.' + ext construction. (Or maybe with template strings, that might look nicer).
| @@ -21,6 +21,6 @@ import replaceStringVariables from './replace-string-variables' | |||
| * | |||
| * @return {String} The final string after replacements. | |||
| */ | |||
| export default function generateId (pattern: string = '%Y%M%D%h%m%s'): string { | |||
| export default function generateId (pattern: string = '%Y%M%D%h%m') { | |||
There was a problem hiding this comment.
Please undo this change, you removed the (for this change unaffected) %s-variable, and removed the return type.
|
Thank you @nathanlesage for the detailed evaluation. Working on it and will update the requested changes. |
Added %ext to FileNamePattern. This is used by function generateFileName, where it replaces %id and %ext.
|
@nathanlesage I have reviewed your suggestions and updated the file. I found that the core issue was in-fact from the config file, where the filenamepattern was set to However, after resolving the merge conflict, I found that current How do you wish me to approach this situation? Do you want me to undo the "merge commit" and force push changes to this branch? |
Description
This PR resolves the issue where new files always defaulted to .md. Now users can create new files with .tex, .json, .yaml, or .md extensions. Unsupported extensions will throw an error in the logs.
Changes
1.Added support for new file types (.tex, .json, .yaml, .md).
2.If an unsupported file extension is chosen, an error is logged.
3.No breaking changes to the existing file creation flow.
Additional information
Relates to #5703
Tested on:
macOS 15.3.2
Node.js v22.14.0
NB: Since this is a bug fix, I wasn’t sure if an entry in CHANGELOG.md is required. Let me know if I should add one, and I’d be happy to update it!