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

Repo cleanup and maintenance #269

Merged
merged 18 commits into from
May 7, 2023

Conversation

YesThatAllen
Copy link
Contributor

@YesThatAllen YesThatAllen commented May 5, 2023

Repo cleanup ref: #265

  • Sort and organize the gitignore
  • introduce .editorconfig
  • Remove old cruft
  • Misc whitespace cleanup
  • Use .keep vs Readme.md to preserve key folders

@YesThatAllen

This comment was marked as resolved.

@mcbirse
Copy link
Collaborator

mcbirse commented May 6, 2023

As part of the clean up, perhaps also delete the file pypowerwall.sample which appears to be an unused extraneous duplicate of pypowerwall.env.sample ?

@YesThatAllen

This comment was marked as outdated.

Copy link
Contributor Author

@YesThatAllen YesThatAllen left a comment

Choose a reason for hiding this comment

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

Some thoughts/questions I had along the way

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.editorconfig Show resolved Hide resolved
influxdb/backups/.keep Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@mcbirse
Copy link
Collaborator

mcbirse commented May 6, 2023

Just on trailing newlines.... there's a few files where it is missing which I find quite irksome! Especially when you do something like below, and end up with the command prompt printing on the same line as the file content.

admin@alpine:~/Powerwall-Dashboard$ cat compose.env.sample
# Environment variables for docker compose.
#
# GRAFANAUSER - may need to be adjusted for cases such as rootless docker.
# See issue #22,
# https://github.com/jasonacox/Powerwall-Dashboard/issues/22#issuecomment-1253076441)
GRAFANAUSER="1000:1000"admin@alpine:~/Powerwall-Dashboard$

I just went through all the files and identified the below with missing trailing newlines:

_config.yml
compose.env.sample
telegraf.local.sample
dashboards\powerflow-animation.html
grafana\README.md
influxdb\README.md
tools\influxdb2\flux\tesla_energy.vitals.flux
tools\solar-only\compose.env.sample
tools\solar-only\grafana\README.md
tools\solar-only\influxdb\README.md
v6.5.1\grafana\README.md
v6.5.1\influxdb\README.md
weather\contrib\ecowitt\Dockerfile

I would recommend adding the trailing newline to those files.

There are more without the trailing newline, but those are .json files so I have ignored them since they shouldn't have the trailing newline? Some do have a trailing newline however so it is a bit inconsistent, but probably doesn't matter too much with those.

@YesThatAllen

This comment was marked as resolved.

@jasonacox
Copy link
Owner

Additionally, I feel like it'd be good to rename sample files so they retain their native extension (.env).. makes it easier for editors to auto-highlight.

Something like renaming pypowerwall.env.sample to pypowerwall.sample.env?

@YesThatAllen

This comment was marked as resolved.

@YesThatAllen YesThatAllen marked this pull request as ready for review May 6, 2023 12:34
@YesThatAllen
Copy link
Contributor Author

ok, I think we're close.

once you're happy with the changes: lmk and I'll rebase these 16 down to like, 4 commits or something something sane for the overall commit history.

@jasonacox
Copy link
Owner

Wow! @YesThatAllen this looks great. Thank you!! 🙇

I can squash and merge unless you think it is better to keep it as ~4 commits.

@mcbirse thanks for your review as well.

@YesThatAllen
Copy link
Contributor Author

Nah, squashing to master as one commit is fine. I just didn't want 16 commits in master lol.

.editorconfig Outdated
Comment on lines 10 to 11
indent_size = 2
indent_style = space
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasonacox / @mcbirse I'm just assuming 2 spaces vs tabs is your preference.

Not every file matches this setting.

The editorconfig does NOT force changes, but does attempt to tell the editor how it should behave when you're adding new code.
Please make sure this is your preference before squashing

Copy link
Owner

Choose a reason for hiding this comment

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

We have a mix, but 4 spaces is my preference (as seen in most of the *py, *sh and *yml) but I know we have 2 spaces in *conf.

Yes, I know, we aren't consistent. This should help. :)
Copy link
Contributor Author

@YesThatAllen YesThatAllen left a comment

Choose a reason for hiding this comment

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

.

.editorconfig Outdated
Comment on lines 19 to 21
# 4 space indentation
[*.py]
indent_size = 4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the default for the repo is 4 spaces, we can remove this fallout.

Not needed since default is already 4 spaces.
@jasonacox jasonacox merged commit 185c62c into jasonacox:main May 7, 2023
@jasonacox
Copy link
Owner

jasonacox commented May 7, 2023

Merged! I'll test and set this up as v2.9.5.

Testing

  • Raspberry Pi - Passed
  • Linux / Ubuntu - Passed
  • Windows 11 - Passed

@jasonacox
Copy link
Owner

@YesThatAllen - Thank you! 🙏

@YesThatAllen YesThatAllen deleted the yesthatallen-265-cleanup branch May 7, 2023 13:29
@Bman7714
Copy link

Less goooo

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.

4 participants