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

SMA Homemanager 2.0 integration #702

Closed
wants to merge 0 commits into from
Closed

SMA Homemanager 2.0 integration #702

wants to merge 0 commits into from

Conversation

Snoopy-HSS
Copy link

Integration SMA Homemanager 2.0

@Snoopy-HSS Snoopy-HSS changed the title Development SMA Homemanager 2.0 integration Feb 27, 2024
Copy link
Author

@Snoopy-HSS Snoopy-HSS left a comment

Choose a reason for hiding this comment

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

main.cpp muss nicht geändert werden, die 2 Zeilen können ignoriert werden.

Copy link
Member

@schlimmchen schlimmchen left a comment

Choose a reason for hiding this comment

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

Scheint so als wäre hier nur das changeset aufzuräumen, der Code sieht gut aus. Wie testet man das?

Copy link
Member

Choose a reason for hiding this comment

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

Änderungen an dieser Datei sollen nicht Teil des Changesets sein.

Copy link
Member

Choose a reason for hiding this comment

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

Änderungen an dieser Datei sollen nicht Teil des Changesets sein.

Copy link
Member

Choose a reason for hiding this comment

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

Änderungen an dieser Datei sollen nicht Teil des Changesets sein.

Copy link
Member

Choose a reason for hiding this comment

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

Änderungen an dieser Datei sollen nicht Teil des Changesets sein.

src/main.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Du schreibst "main.cpp muss nicht geändert werden, die 2 Zeilen können ignoriert werden.". Warum? Für mich sieht das richtig und gewollt aus?

@Snoopy-HSS
Copy link
Author

Snoopy-HSS commented Mar 5, 2024 via email

@Snoopy-HSS
Copy link
Author

Ein Software SMA Homemanger zum testen hier:

https://github.com/RalfOGit/sma-emeter-simulator

@Snoopy-HSS Snoopy-HSS requested a review from schlimmchen March 5, 2024 11:40
@schlimmchen
Copy link
Member

Interesting, even though I changed my wording you also did the same wrong thing as Gumbagubanga in #706 😉

Do not delete the whole files (main.cpp and platformio_override.ini), revert your changes to them, so they appear untouched by this PR. Like git fetch helgeerbe; git checkout helgeerbe/development -- main.cpp; git commit -m "revert main.ccp" or similar.

@Snoopy-HSS Snoopy-HSS closed this Mar 5, 2024
@Gumbagubanga
Copy link

Interesting, even though I changed my wording you also did the same wrong thing as Gumbagubanga in #706 😉

Do not delete the whole files (main.cpp and platformio_override.ini), revert your changes to them, so they appear untouched by this PR. Like git fetch helgeerbe; git checkout helgeerbe/development -- main.cpp; git commit -m "revert main.ccp" or similar.

I think this information (and maybe more in the currently missing contribution-section?) should be added to the upstream project. The main documentation is not clear about that. :-/

@schlimmchen
Copy link
Member

I am confused... @Gumbagubanga you just now told me in a different context that you have discussions at work about code style. That means you have some sort of professional background with programming. Why is this sort of issue giving you trouble? What should be written in the contribution guideline? "Do not delete files from the repo"?

Are you suggesting a list of files not to commit changes to, maybe?

@Gumbagubanga
Copy link

At work we have a "no generated source code in the repository" policy and directories like webapp_dist wouldn't be found as they'd be placed in .gitignore like the .pio directory in this project.

I honestly haven't given much thought on that here, as I assumed that the CI/CD pipeline either uses the user-generated package (bad practice, but not my decision) or overwrites the file for the release.
It's me being a bit too careless for my own liking. I won't do that mistake again, but I understand that no one wants to repeat the lecture on which files to change and which not over and over again.

So I'm suggesting putting the webapp_dist directory in the .gitignore as it was done for .pio directory.

@schlimmchen
Copy link
Member

A quick follow-up: As the webapp_dist/ files are currently tracked, adding them to .gitignore will not help much, as changes will still be tracked. We would need to convince the upstream project to not version the webapp_dist files any longer, i.e., to remove them from the repo. As I don't think that's going to happen, there is no use in adding webapp_dist/ to git's ignore list.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants