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

Cosmetic replace 'fan_only' string #1604

Conversation

PtilopsisLeucotis
Copy link
Collaborator

Hi

This is cosmetic fix, that replace string 'fan_only' to 'Fan Only', for better visual consistency

@crankyoldgit
Copy link
Owner

The reason "Fan_Only" is used, instead of "Fan Only" is because Google Home + Home Assistant use that keyword.

We should add a note to this affect and possibly think of adding "Fan Only" as an addition.

@crankyoldgit crankyoldgit self-assigned this Sep 19, 2021
@PtilopsisLeucotis
Copy link
Collaborator Author

Now in code used string in lower case ('fan_only'). If we cannot use "Fan Only" string, may be better to use "Fan_Only" string (capitalized)?

Copy link
Collaborator

@NiKiZe NiKiZe left a comment

Choose a reason for hiding this comment

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

We will probably not merge this, but instead we should just try to add documentation to fan_only to explain why it is as it is.

However let me take the opportunity to say that if we where to consider this PR then it must be without the changes to doxygen, since that is generated and merged during release process, if any changes is made directly to the files we are more likely to get unnecessary merge conflicts during the release process.

@crankyoldgit
Copy link
Owner

Now in code used string in lower case ('fan_only'). If we cannot use "Fan Only" string, may be better to use "Fan_Only" string (capitalized)?

The string gets used without depending on case.
See:

!strcasecmp(str, kFanOnlyStr))

E.g. fAN, FaNonLy, & fAn_OnLy etc all work if you check the surrounding code.

So, I don't think we really need this PR. The only reason "only" is included is to handle systems that use it for some bizarre reason.
We do need to add a note though. Thanks for highlighting it.

@PtilopsisLeucotis
Copy link
Collaborator Author

PtilopsisLeucotis commented Sep 19, 2021

So, I don't think we really need this PR. The only reason "only" is included is to handle systems that use it for some bizarre reason.
We do need to add a note though. Thanks for highlighting it.

The main reason, why I proposed this PR is better visual perception of log records in web console in Tasmota.
This is how currently log record look:
..."Vendor":"HAIER_AC_YRW02","Model":-1,"Mode":"Dry","Power":"On"...
..."Vendor":"HAIER_AC_YRW02","Model":-1,"Mode":"fan_only","Power":"On"....
It would be more consistently and more neat with if log records look as:
..."Vendor":"HAIER_AC_YRW02","Model":-1,"Mode":"Fan_Only","Power":"On".... or
..."Vendor":"HAIER_AC_YRW02","Model":-1,"Mode":"Fan Only","Power":"On"....

@PtilopsisLeucotis
Copy link
Collaborator Author

However let me take the opportunity to say that if we where to consider this PR then it must be without the changes to doxygen, since that is generated and merged during release process, if any changes is made directly to the files we are more likely to get unnecessary merge conflicts during the release process.

I removed all my changes in doxygen

crankyoldgit added a commit that referenced this pull request Sep 21, 2021
* Improve compatiblity and human clarity.
* Add note about why it exists in the first place.

Replaces PR #1604
@crankyoldgit
Copy link
Owner

Closing as I've reworked it as PR #1610

@PtilopsisLeucotis PtilopsisLeucotis deleted the patch_fix_fan_only branch September 21, 2021 17:20
crankyoldgit added a commit that referenced this pull request Sep 21, 2021
* Improve compatiblity and human clarity.
* Add note about why it exists in the first place.

Replaces PR #1604
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants