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

Add Internal Temperature usermod #3246

Merged
merged 4 commits into from
Sep 1, 2023
Merged

Conversation

dima-zhemkov
Copy link
Contributor

No description provided.

@softhack007
Copy link
Collaborator

Is this the usermod from @lost-hope, or is it different?
MoonModules#51

@dima-zhemkov
Copy link
Contributor Author

Is this the usermod from @lost-hope, or is it different? MoonModules#51

It's based on corresponding usermod from MoonModules.

@softhack007 softhack007 self-requested a review June 13, 2023 10:30
Copy link
Collaborator

@softhack007 softhack007 left a comment

Choose a reason for hiding this comment

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

Please clarify code ownership with @lost-hope, who created the same usermod in the MM fork. I would prefer to have clear attribution of the code origin.

@blazoncek
Copy link
Collaborator

If this is the case of duplicate PR in different forks, it exactly shows why I prefer authors make PRs into upstream first.
If the PR is rejected then PR into other forks.
/cc @softhack007 @lost-hope

@lost-hope
Copy link
Collaborator

@blazoncek i PRed it into MM since it was requested by someone there. So why make a PR in AC, when it then has to be cherry picked into MM, when i could deliver it directly to MM for him.

@dima-zhemkov it is ok here (not a lot of special and hard code in it), but you should ask the creator first instead of "getting caught.

@softhack007 see above ;D

@blazoncek
Copy link
Collaborator

So why make a PR in AC, when it then has to be cherry picked into MM

Cherry picking only happens if there are conflicts or the maintainer doesn't want certain commits/code in their fork.

@dima-zhemkov
Copy link
Contributor Author

@lost-hope I haven't "got caught" since I haven't broken any agreement. MIT license allows me to copy/reuse/modify any code from MoonModules/WLED repository without any additional permission of authors of that code as soon as a copy of the MIT license is in a destination repository. And I copied, literally, 5 lines of your code.
@softhack007 why did you ask me to clarify code ownership if you knew that it's under MIT license?

@softhack007
Copy link
Collaborator

softhack007 commented Jun 13, 2023

@softhack007 why did you ask me to clarify code ownership if you knew that it's under MIT license?

@dima-zhemkov It could have been that @lost-hope already agreed with you for offering his work to upstream, and you actually agreed to maintain the code he created. That's what I did not know so I asked for clarification of ownership.

I'm not a layer, so let me say it this way: there is a difference between "what's not forbidden" vs. "what is the right thing to do". Instead of doing whatever is legally possible, in open source its important to respect the people who give their source code for free, knowing that there is no copyright, license fees or patents. Even when MIT looks like "free beer", it is not.

For me, taking source code from someone else (and its not just 5 lines, its >95% of total lines of code), without giving credit to the author, is not the right way. In open source we are very interested to know "where is it from". In the end, Its a matter of "code of conduct". In that sense, yes you got caught for violating "implied rules". I hope you'll do better next time.

Technically, now that we have the statement from @lost-hope, I understand he is agreeing that you submit your work and his part together, and we can proceed with this PR.

@dima-zhemkov
Copy link
Contributor Author

dima-zhemkov commented Jun 13, 2023

@softhack007 so I should have merely mention name of the author in a description?

Copy link
Collaborator

@softhack007 softhack007 left a comment

Choose a reason for hiding this comment

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

See detailed code comments.

@softhack007 softhack007 added waiting for feedback addition information needed to better understand the issue usermod usermod related labels Aug 13, 2023
@dima-zhemkov dima-zhemkov reopened this Aug 25, 2023
@blazoncek
Copy link
Collaborator

As far as I am concerned this is ok.
@lost-hope do you agree with this PR? I see you are credited.
@softhack007 do you have any more objections?

@lost-hope
Copy link
Collaborator

Fine with me

Copy link
Collaborator

@softhack007 softhack007 left a comment

Choose a reason for hiding this comment

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

Somehow my last review was lost after force-push 🙄 by @dima-zhemkov

Sometimes commits can disappear after a force-push.

Ok for me with the latest changes.

Seems that the function for reading -S3 or -C3 sensors was lost? Well so be it - could be added after merge.

@softhack007 softhack007 merged commit 822dd24 into Aircoookie:main Sep 1, 2023
11 checks passed
@blazoncek blazoncek removed the waiting for feedback addition information needed to better understand the issue label Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usermod usermod related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants