Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Move scripts to Synapse package and expose as entry_points; add suffices to scripts-dev #12107

Closed
DMRobertson opened this issue Feb 28, 2022 · 8 comments
Assignees
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@DMRobertson
Copy link
Contributor

DMRobertson commented Feb 28, 2022

black and flake8 will ignore such files by default. flake8 has a filename config option, but I haven't been able to use this to have flake8 run over the scripts it would otherwise script.

mypy does not have this problem: one can specify an explicit list of files to example or ignore. isort doesn't have such configuration, but does look for shebang lines to detect python scripts without a .py suffix.

Given a magic wand, I would like to:

  • rename all scripts to end in .py
  • change the documentation to include the new .py suffix
  • change the debian package build so that the new scripts are available by the old, suffixless filename

Bullet three here is the hard one.

This would allow me to run black . and flake8 . both locally and in CI without thinking.

@DMRobertson DMRobertson added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Feb 28, 2022
@DMRobertson
Copy link
Contributor Author

  • change the debian package build so that the new scripts are available by the old, suffixless filename

Bullet three here is the hard one.

Aha! This is done in matrix-synapse-py3.links.

@richvdh
Copy link
Member

richvdh commented Feb 28, 2022

I am not enthusiastic about naming commands according to the language they happen to be written in. It feels a bit like Hungarian notation. Linux prefers #! lines over file extensions for a reason.

However, Setuptools allows an entry point to be defined in a file named *.py and still get installed into the virtualenv with its regular name - see https://setuptools.pypa.io/en/latest/userguide/entry_point.html.

It looks like poetry's scripts do much the same thing.

@DMRobertson
Copy link
Contributor Author

An alternative would be to keep passing around the list of explicit targets to lint, both in CI and in lint.sh. That's fine and is probably easier---but it would mean maintaining duplicate lists, which is the kind of thing I'm trying to move us away from with poetry.

On entry_points and scripts: I'm glad you brought that up. When I looked at register_new_matrix_user I thought "this could just be an entry point".

from synapse._scripts.register_new_matrix_user import main
if __name__ == "__main__":
main()

I think this script was moved to within Synapse (#4085) in order to be tested here:

from synapse._scripts.register_new_matrix_user import request_registration

How would you feel about migrating the scripts' source code to synapse._scripts and exposing them as entry_points? It'd be useful for #11537, because I don't think poetry's config will offer me a way to glob within the scripts directory like we have at present:

scripts=["synctl"] + glob.glob("scripts/*"),

@richvdh
Copy link
Member

richvdh commented Mar 1, 2022

How would you feel about migrating the scripts' source code to synapse._scripts and exposing them as entry_points?

sgtm

@DMRobertson DMRobertson changed the title Rename all scripts to end in .py Move scripts to Synapse package and expose as entry_points. Mar 1, 2022
@DMRobertson DMRobertson self-assigned this Mar 1, 2022
@DMRobertson
Copy link
Contributor Author

In addition to #12118 I need to do this for synctl.

I'd also like to add the .py suffix within scripts-dev. I sympathise with "things to be executed shouldn't have a suffix", but

  • we're not distributing those scripts with installations
  • most of them have a suffix anyway
  • it will let me prune a lot of boring CI config

@richvdh
Copy link
Member

richvdh commented Mar 2, 2022

yeah I'm less fussed about scripts-dev. It's a hodgepodge of "expert" stuff anyway.

@DMRobertson DMRobertson changed the title Move scripts to Synapse package and expose as entry_points. Move scripts to Synapse package and expose as entry_points; add suffices to scripts-dev Mar 2, 2022
@richvdh
Copy link
Member

richvdh commented Mar 10, 2022

@DMRobertson can we call this done?

@DMRobertson
Copy link
Contributor Author

Yes. I think it just didn't get closed because the PR was targetted at a release branch.

DMRobertson pushed a commit that referenced this issue Mar 30, 2022
After #12107 it's much easier for black, isort and flake8 to find the
scripts we want them to lint.
DMRobertson pushed a commit that referenced this issue Mar 30, 2022
After #12107 it's much easier for black, isort and flake8 to find the
scripts we want them to lint.
DMRobertson pushed a commit that referenced this issue Mar 30, 2022
After #12107 it's much easier for black, isort and flake8 to find the
scripts we want them to lint.
DMRobertson pushed a commit that referenced this issue Apr 8, 2022
instead, configure the tools to find sensible targets.

Pulled out from #12337; part (but not all) of
e53e99e. Related: #12107.
DMRobertson pushed a commit that referenced this issue Apr 11, 2022
- We don't recommend tox in `code_style.md`
- After #12107 it's much easier for black, isort and flake8 to find the
  scripts we want them to lint; we don't need to maintain `lint_targets`
  in the tox config.
DMRobertson pushed a commit that referenced this issue Apr 11, 2022
- We don't recommend tox in `code_style.md`
- After #12107 it's much easier for black, isort and flake8 to find the
  scripts we want them to lint; we don't need to maintain `lint_targets`
  in the tox config. See also #12420.
DMRobertson pushed a commit that referenced this issue Apr 11, 2022
- We don't recommend tox in `code_style.md`
- After #12107 it's much easier for black, isort and flake8 to find the
  scripts we want them to lint; we don't need to maintain `lint_targets`
  in the tox config. See also #12420.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

No branches or pull requests

2 participants