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

EditorConfig: Use four-space indents for Python scripts #3169

Closed
wants to merge 1 commit into from

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Oct 12, 2018

Changes proposed:

(What are you proposing we change?)

Steps to replicate the issue:

(If this PR is not fixing a defect/bug, you can remove this section.)

  1. Edit a Python script
  2. Press tab
    -> Adds two spaces

Steps to verify the solution:

(How does someone verify that this does what you think does?)

  1. Edit a Python script
  2. Press tab
    -> Adds four spaces

@danepowell
Copy link
Contributor

Are python scripts commonly used with some particular Drupal or BLT feature? I've never seen python files in a Drupal codebase, most people use PHP (for obvious reasons 😄 )

There are thousands of scripting languages out there and BLT can't accommodate them all. I know this is a pretty tiny change, but I think it still needs to be justified as to why Python is special.

@hugovk
Copy link
Contributor Author

hugovk commented Oct 26, 2018

We're using Python scripts as a tool to do extra stuff on the CI, processing Lighthouse output and sending it to some other channels.

I understand this is not really BLT's worry, if we want to use a thousand scripting language, that's our choice and our problem :)

I guess the root problem is that BLT replaces any edits we make to .editorconfig.

Is there a way we can override it and avoid BLT replacing it?

Thank you!

@mikemadison13 mikemadison13 added the Enhancement A feature or feature request label Nov 10, 2018
@mikemadison13
Copy link
Contributor

@hugovk when you say BLT overwrites the .editorconfig file, that only happens during blt updates, right? you should be able to modify the file, commit your version, and then allow blt to overwrite it during updates (and diff what blt wants vs. what you had previously to keep the changes). or is there something else going on?

@hugovk
Copy link
Contributor Author

hugovk commented Nov 10, 2018

Yes, something like that.

Currently, we modify and commit it. The next BLT update overwrites it so we either have to lose our changes, or manually fix it again. We don't want to re-fix things.

I'd rather have our version not overwritten and forget about the BLT update to this particular file, so we can maintain code style in our codebase without worry.

@danepowell
Copy link
Contributor

Making it easier to manage the editor config file on a per-project basis seems like a better solution here than adding one-off rules for languages (i.e. Python) to BLT.

@mikemadison13
Copy link
Contributor

a couple of additional thoughts here:

we shouldn't change the default editor config file for this, so this PR needs to close. HOWEVER the use case presented here is 100% valid, and there are 2 paths forward, 1 immediate and 1 longer term.

the immediate fix is to define your own ignore list

ignore-existing-file: ${blt.root}/scripts/blt/ignore-existing.txt
in your blt.yml file, and add editor config to that, so blt stops updating that file.

the longer term fix is to add this file to the blt global ignore list by default.

@hugovk hugovk deleted the patch-1 branch December 17, 2018 18:06
grasmash pushed a commit that referenced this pull request Dec 21, 2018
Fixes #3169 
--------

Changes proposed:
---------
(What are you proposing we change?)

- adds the .editorconfig file to the ignore list for blt updates
mikemadison13 added a commit to mikemadison13/blt that referenced this pull request Feb 21, 2019
Fixes acquia#3169 
--------

Changes proposed:
---------
(What are you proposing we change?)

- adds the .editorconfig file to the ignore list for blt updates
mikemadison13 added a commit to mikemadison13/blt that referenced this pull request Feb 21, 2019
Fixes acquia#3169 
--------

Changes proposed:
---------
(What are you proposing we change?)

- adds the .editorconfig file to the ignore list for blt updates
mikemadison13 added a commit that referenced this pull request Feb 25, 2019
* Fixes #3265 to expand alias documentation. (#3399)

* Fixes #3401 for 9.2.x to update devel.

* Moved pipelines-deploy from build event to post-deploy event (#3374)

* Docksal documentation (#3268)

* Add links to docsal and ddev pages.

* Updates ignore-existing to include .editorconfig. (#3300)

Fixes #3169 
--------

Changes proposed:
---------
(What are you proposing we change?)

- adds the .editorconfig file to the ignore list for blt updates

* Updating BLT's docs to remove nvm and point to Cog's documentation for theme installation. (#3313)

* Fix broken link to Cloud Hooks docs. (#3331)

* Added steps to install nfs-utils and enable access via firewalld; without it DrupalVm gets stuck at mounting NFS (#3376)

* Updates release and readme docs. (#3382)

* Updates Travis and Pipelines to PHP 7.1 by default.

* Adds FAQ for drush 8/9 permission denied issue. (#3264)

* Add setup instructions for PHPStorm + PHPUnit (#1787) (#3198)

* Update INSTALL.md (#3273)

* Speed up deploys with shallow fetching (#3271)

Before:
```
real	4m11.340s
user	1m3.430s
sys	1m23.638s
```

After:
```
real	3m14.776s
user	0m47.570s
sys	1m6.415s
```

* Fixes #3217 to provide acsf-init-verify as part of git hooks and blt validate. (#3218)

* Updating Drupal VM config versions. (#3328)

* Fixes #3345 to run db updates even when config strategy is none. (#3363)

* Fixes #3362 to better identify why the site URI is not found. (#3369)

* Test tests:acsf:validate (#3392)

* Test tests:acsf:validate

* Delete README.md

Remove cloud hook readme.

* Update AcsfHooksTest.php

* Update AcsfHooksTest.php

* Update DeployTest.php

* Fixes #3383 to backport Pipelines update to 9.2.x.

* Increase wait timeout. (#3163)

* Fixes #3122 to remove misleading jmeter example. (#3230)

* Fixes #3223 to update git docs. (#3226)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement A feature or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants