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

Use string.Template when creating a new file #21706

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

sphh
Copy link
Contributor

@sphh sphh commented Jan 14, 2024

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Spyder uses a file template when opening a new file. In this template known tokens are replaced by values. Currently only two tokens are recognized: %date and %username. The syntax %<name> also interferes with more elaborated templates (e.g. #21058).

This PR now changes the simple text % VARS replacement with the string.Template functionality built into Python. This now uses $date (or ${date}) instead and the dollar signs can be escaped. This should help in writing more elaborated customary templates.

Also more template fields are made available with this PR, mainly related to the current time. But also the full name of the user is made available under $fullname. These file related variables are also available: $file, $filename, $filepath and also these project related variables: $projectname, $projectpath.

Finally $|$ can be used to mark the position of the cursor in the new file!

It also adds a Copyright ${year} ${fullname} line to the default template (and the @file and @project key-value pairs) and uses the datetime.datetime module instead of time (because that is imported anyway for newer versions of this file).

Caveats

It breaks the template syntax, because it uses $ instead of % to mark variables in the template. On the other hand it resolves the problem due to using %.

The update is easy: Replace all %<name> with $<name> in your template.

Note: When backporting this PR to Spyder 5, please add the following import statement from datetime import datetime and remove import time from the file.

Issue(s) Resolved

Fixes #21058
Fixes #11426
Fixes #6379
Fixes #2884 (That issue consists actually of two issues: One regarding the template itself – that is fixed – and one regarding its documentation and move the edition to the settings dialog – that is not fixed)

Closes #14579

Question

The current default template includes a # -*- coding: utf-8 -*- line. Is this still necessary? Python already uses utf-8 throughout …

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: sphh

…mplate.

- `${year}`: The current year with century as a decimal number.
- `${month}`: The current month as a decimal number [01,12].
- `${monthname}`: The current month's (local) name.
- `${day}`: The current day of the month as decimal number [01,31].
The new `plugin.py` imports `datetime`, so we use it.

IMPORTANT: When backporting, add `from datetime import datetime`!
…le template.

We now have the following variables, which can be used in the new file
template:
- `${date}`: The date and time without timezone information.
- `{$isodate}`: The date and time in ISO format (including timezone
information).
- `${year}`: The current year with century as a decimal number
[0001,9999].
- `${month}`: The current month as a zero-padded decimal number [01,12].
- `${day}`: The current day of the month as zero-padded decimal number
[01,31].
- `${hour}`: The current hour (24-hour clock) as a zero-padded decimal
number [00,23].
- `${minute}`: The current minute as a zero-padded decimal number
[00,59].
- `${second}`: The current second as a zero-padded decimal number
[00,59].
- `${tzname}`: The local time zone name.
- `${monthname}`: The current month as locale's abbreviated name.
- `${weekday}`: The current weekday as locale's abbreviated name.
Uses the OS name instead of trying to receive the 'USERNAME' environment
variable, which is only available on Windows.
…on of the new file name.

This is needed, so that we can include a filename and filepath variable
to be used in the new file template.
… the new file template.

New template variables:
- `${file}` - The full file name including the path.
- `${filename}` - The file name.
- `${filepath}` - The file path.
… in the new file template.

New template variables:
- `${projectname}` - The project name.
- `${projectpath}` - The project directory.
The first occurance of the marker `$|$` is removed from the template and
the cursor positioned in that place.
@sphh
Copy link
Contributor Author

sphh commented Jan 14, 2024

I have update my original description with the new features of my last commits. I hope that's ok.

@ccordoba12 @CAM-Gerlach:

  • Please feel free to modify the default template to your liking (or tell me, what you prefer, so that I can include that in a commit)!
  • Please also tell me, if you would prefer different names for the template variables.
  • For some variables ($fullname, $projectname and $projectpath) I use "n/a", if those variable are empty. That's debatable if this is a good choice. Should it be translated? Please advise and I am happy to modify it.
  • Finally, if you like more variables to be included, please feel free to tell me and I will look what I can do.

@sphh
Copy link
Contributor Author

sphh commented Jan 15, 2024

The failing tests puzzle me. If I understand the failed test test_go_to_prev_next_cursor_position correctly, the movement of the cursor in the third file should not result in this file being appended to the history.

My guess is, that test testing the last entry of the history stack ([-1]) fails. This test should test that the third file file3.py is not added to the history (and the newly created untitled5.py is still the last entry in the history).

Is this correct?

If yes, what could I do to pass the tests, because I believe the error is outside of my PR?
If not, what have I missed?

Thank you so much for your help!

@CAM-Gerlach CAM-Gerlach self-assigned this Mar 10, 2024
@CAM-Gerlach CAM-Gerlach changed the title Uses string.Template when creating a new file. Use string.Template when creating a new file Mar 10, 2024
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Hey, thanks for tackling this, @sphh — its a great start and a big improvement!

I did have some comments, mostly as suggestions; as usual, you can apply the ones you want (or all of them) by going to the Files changed tab, clicking Add to batch for each one, and then Commit once you've selected all the ones you want.

My suggestions also need some additional imports: getpass from the stdlib, and adding timezone to from datetime.

spyder/plugins/editor/plugin.py Show resolved Hide resolved
Comment on lines +214 to +217
'@file: ${file}',
'@project: ${projectname}',
'',
'Copyright ${year} ${fullname}',
Copy link
Member

Choose a reason for hiding this comment

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

IMO, I'm not sure how much value these fields really add by default, at least to the header (as opposed to in a user's own custom template)? IIRC, years ago we simplified this quite a bit because users complained that it was too long. And as to the specific fields:

  • I'm not sure in what scenario simply repeating the (original) file name is ever really helpful. Won't it just be untitledN.py at creation time when this is generated? And even if not, given its already displayed in the editor, file manager, etc. as the canonical, up to date source of truth, and is the name of the module when imported or the script when run, and likely to drift out of date if it changes, I don't see how it adds value to duplicate the information here.
  • The project name could perhaps theoretically have a small amount of utility, but only if users use projects, their project directory name is meaningful, and they don't create/change it after file creation. Furthermore, it is visible in the Spyder UI and from the file path, so it only adds anything extra in the case they share the file with other users, in which case it's unlikely to mean much anyway.
  • The copyright year duplicates the year in the created on, the name (mostly, if not completely) duplicates the username, and the copyright notice is no longer legally required for copyright protection and in many cases be inaccurate, e.g. in many jurisdictions, copyright is held by universities in many cases for the work of students and professors, and by employers for employees.

I could certainly see them making sense in some user-created templates, but I'm not seeing the case for adding them by default. Therefore:

Suggested change
'@file: ${file}',
'@project: ${projectname}',
'',
'Copyright ${year} ${fullname}',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that's to everyone's liking. That's why we have templates, don't we?

Personally I always find it easier to delete tokens than to look up, which tokens are available. That's why I added maybe to many to the default?

There are three reasons why I added ${file} and ${projectname}: Firstly, sometimes I move around or rename files inside a project or even split a project into two. If you were to do it with out using git, it is so much easier to move them back to their original place. Secondly, files published tend to appear all over the Net and you never know, where they originally come from. This information might help (mind you, it's not foolproof, because this information can be deleted). Thirdly, I once accidentally deleted a bunch of files. Recovering them gave me their contents, but not their name or place in the directory tree.

Re the copyright notice: If someone reused your file in one of his/her projects, the copyright notice often gets lost; often not of malice, but because it is cumbersome to track down the original copyright holder and insert it into the file's header. Including the copyright notice per default makes this in my view so much easier.

What do you think? I do not have a strong feeling to remove them, but they might be useful. What do the users say? You probably know that better than I do.

spyder/plugins/editor/plugin.py Show resolved Hide resolved
spyder/plugins/editor/plugin.py Show resolved Hide resolved
spyder/plugins/editor/plugin.py Show resolved Hide resolved
spyder/plugins/editor/plugin.py Show resolved Hide resolved
'monthname': now.strftime('%b'),
'weekday': now.strftime('%a'),
'username': username,
'fullname': displayname or 'n/a',
Copy link
Member

Choose a reason for hiding this comment

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

Here, I'd suggest just falling back to the username if there is no separate display name defined, as it seems much more sensible behavior for realistic use cases, and avoids the need to handle (and translate) a default.

Suggested change
'fullname': displayname or 'n/a',
'fullname': displayname or username,

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 also considered to use username as alternative, if displayname is not available. I decided against it, because I believed that it might come to a surprise, if one uses fullname and suddenly just the displayname is inserted. Maybe we should have username, fullname and full_or_username (or something similar)?

spyder/plugins/editor/plugin.py Show resolved Hide resolved
Comment on lines +2336 to +2337
'projectname': project_name or 'n/a',
'projectpath': project_path or 'n/a'
Copy link
Member

Choose a reason for hiding this comment

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

Not totally sure what to do about these in the case of no project being active. For starters, we should use proper capitalization, and calling _() will translate the string, i.e.

Suggested change
'projectname': project_name or 'n/a',
'projectpath': project_path or 'n/a'
'projectname': project_name or _('N/A'),
'projectpath': project_path or _('N/A'),

We could just fall back to getcwd_or_home(), as Spyder does for the default working dir when no project is active. i.e.:

Suggested change
'projectname': project_name or 'n/a',
'projectpath': project_path or 'n/a'
'projectname': project_name or _('N/A'),
'projectpath': project_path or getcwd_or_home(),

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 am used to both ways: ‘n/a’ and ‘N/A’. When working in the UK I got the feeling, that ‘n/a’ was more common. So does www.leo.org only shows ‘n/a’. On the other hand, wikipedia has an entry for ‘N/A’. I don't have any preferences here.

I prefer to show, that the file is not part of a project instead of using the current working directory or even home. Otherwise one might think, that the file is part of a project, which it is not.

spyder/plugins/editor/plugin.py Show resolved Hide resolved
@sphh
Copy link
Contributor Author

sphh commented Mar 11, 2024

@CAM-Gerlach I marked a couple of proposed changes with Add to batch and then committed those. Strangely I got an error message. I cannot find the commit and I cannot add those changes again to the batch. Could you please check, if those changes were actually committed? Thanks.

@sphh
Copy link
Contributor Author

sphh commented Aug 19, 2024

@CAM-Gerlach: Bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants