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

Tweak Python icons #85

Closed
wants to merge 1 commit into from
Closed

Tweak Python icons #85

wants to merge 1 commit into from

Conversation

mataha
Copy link

@mataha mataha commented Dec 23, 2023

Description

This commit:

  • adds PyPI icon to related files (requirements.in, requirements.txt)
  • adds a modified Python icon to py.typed files
  • adds an icon separate from Python to .python-version files

Motivation and Context

Improved visual grepping in Python projects.

How Has This Been Tested?

  • py.typed
  • requirements.in
  • requirements.txt

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have read the CONTRIBUTING document.

icons/icons.json Outdated Show resolved Hide resolved
icons/icons.json Outdated
".python-version"
],
"name": "Plain Text (Python)",
"scope": "text.plain.python"
Copy link
Member

Choose a reason for hiding this comment

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

Two definitions with same scope doesn't work. Also do we really need dedicated icons for every python related meta-data file?

Copy link
Author

Choose a reason for hiding this comment

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

I certainly do not think so, but I wouldn't consider these "every python related meta-data file" - just the bare minimum expected in a reasonably-sized Python project and tied to the tools included with the runtime.

Copy link
Author

Choose a reason for hiding this comment

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

Changed to extend source.python instead.

Copy link
Author

Choose a reason for hiding this comment

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

Actually I'm not too convinced myself that changing .python-version is a good idea given that the icon has been present for quite some time and people have gotten accustomed to it - reverting...

Copy link
Member

@deathaxe deathaxe left a comment

Choose a reason for hiding this comment

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

Well, except requirements.txt most other python related meta data or configuration files are just INI, TOML or empty text files.

I am not yet convinced of adding a dedicated icon for each file type, actually.

Comment on lines +2604 to +2639
"base": "source.python",
"extensions": [
"py.typed"
],
"name": "Python (PEP 561)",
"scope": "source.python.typed"
Copy link
Member

Choose a reason for hiding this comment

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

It's just an empty marker file, isn't it? Hence creating an alias from a full python syntax is waste of resources.

Suggested change
"base": "source.python",
"extensions": [
"py.typed"
],
"name": "Python (PEP 561)",
"scope": "source.python.typed"
"extensions": [
"py.typed"
],
"name": "Python (PEP 561)",
"scope": "text.python.typed"

Copy link
Author

@mataha mataha Jan 4, 2024

Choose a reason for hiding this comment

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

I've seen some projects include Python-style comments within, though that's not a rule.

(That's one of the reasons I opted for a monochrome Python icon; another is familiarity with Atom icons.)

Comment on lines +2308 to +2330
"requirements.in",
"requirements.txt"
Copy link
Member

Choose a reason for hiding this comment

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

How about the requirements-dev.txt?

Copy link
Author

Choose a reason for hiding this comment

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

I wish this was standardised, but instead we have:

Sublime doesn't offer an easy way out; do I have to include every possible permutation here?

icons/icons.json Outdated
],
"color": "blue"
},
"file_type_python_version": {
Copy link
Member

Choose a reason for hiding this comment

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

The associated icon is not dedicated to a versions but looks more like generic "python settings" or "python config".

It would therefore also fit for .flake8, pyproject.toml or any other python related configuration file.

However, it's probably not feasable to override all file types like TOML or INI with application specific icons as it causes type information to become augmented, but maybe there are some more which could use this icon.

As such a more common name (e.g.: file_type_python_settings or file_type_settings_python) would probably be more suitable.

Actually I'd probably assign all meta files like py.typed, .python-version and maybe special purpose files such as .flake8 to a single icon if we want to distinguish them from normal python sources - maybe even requirements.txt.

Copy link
Author

Choose a reason for hiding this comment

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

I agree regarding .flake8 et al. Is the new definition fine?

This commit:

 - adds PyPI icon to related files (`requirements.in`, `requirements.txt`)
 - adds a modified Python icon to `py.typed` files
 - adds a Python settings hybrid icon to Python-related configuration files
@deathaxe deathaxe closed this Apr 7, 2024
@mataha mataha deleted the feat/python branch May 18, 2024 17:58
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.

2 participants