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

Make metaData function static #2016

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make metaData function static #2016

wants to merge 3 commits into from

Conversation

jano42
Copy link
Contributor

@jano42 jano42 commented Nov 28, 2019

Change MetaDataField methods as static. (functions to convert MetaDataField to/from string).
This allow an easier usage of MetaDataField (calling code can now have the name of the MetaDataField and avoid a duplicated declaration)

@jano42
Copy link
Contributor Author

jano42 commented Nov 28, 2019

@Fishwaldo : check fails from an unknown reason for me

@Fishwaldo
Copy link
Member

Failing checks are known issue right now. (Github Actions are nerfed for pull requests unfortunately).

I don’t quite follow your reasoning here. Can you show me a code sample how this helps.

I’m also weary of breaking existing apps (we try to stay API stable) and not sure how flicking to static would impact apps that are calling from the manager instance instead.

@Fishwaldo Fishwaldo self-assigned this Nov 28, 2019
@jano42
Copy link
Contributor Author

jano42 commented Nov 29, 2019

In fact here is my code:

void COpenZWaveController::getNodeMetaData(uint32 homeId, uint8 nodeId, shared::CDataContainer& result)
{
   for (int i = 0; i < OpenZWave::Node::MetaData_Invalid; ++i)
   {
      try
      {
         const auto mdi = static_cast<OpenZWave::Node::MetaDataFields>(i);
         std::string metaName = OpenZWave::Node::GetMetaDataString(mdi);
         if (metaName.empty())
         {
            i = OpenZWave::Node::MetaData_Invalid;
         }
         else
         {
            std::string metaValue = OpenZWave::Manager::Get()->GetMetaData(homeId, nodeId, mdi);
            if (!metaValue.empty())
            {
               result.set(metaName, metaValue);
            }
         }
      }
      catch(...)
      {
         break;
      }
   }
}

The aim : collecting all the node metaData without specifying each entry. => in case of OZW evolution with new meta, nothing to do for me. Then another reason is to get the metaData name, I don't wnat to add the string to my code, has it is already defined in your.

The last reason for making those methods static, is that they are "static" ! (no interaction with class instance,... => my resharper tools recommandation told me to make it static)

Finaly adding static, just allow other code to use it without getting node instance (OpenZWave::Node::GetMetaDataString() ), and then adding static do not change anything to current code.

@Fishwaldo
Copy link
Member

Ahhhh.

So we are actually trying to move away from a app having to access any class other than Manager (Node is a hangover due to some of these ENUM's though). So My preference would be to add a Method to the Manager Class to retrieve the metadata names, and that can call into the static Node Method to get it

If you could add a method to Manager.h/cpp I'll merge the PR.

@jano42
Copy link
Contributor Author

jano42 commented Nov 29, 2019

Ok you're right. This is a good idea to have only Manager class as public api.
I've added methods as requested.

jano42 and others added 2 commits December 2, 2019 07:45
Convert function (to manage MetaDataField to/from Int) has static
@petergebruers
Copy link
Collaborator

@jano42 typo I guess:

metaDataFiledtoParse -> metaDataFieldtoParse

@Fishwaldo
Copy link
Member

I put some comments. Spelling Errors there :)

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