search

neoave / mrack

Multicloud use-case based multihost async provisioner for CIs and testing during development
Apache License 2.0
11 stars 14 forks source link

handle non-json file for get_data function #287

Closed skycastlelily closed 7 months ago

skycastlelily commented 7 months ago

Hi @pvoborni , thanks for your approval for the workflow, I have updated the commit message just now:)

skycastlelily commented 7 months ago

Hi, The original get_data's function is: return dict parsed from json file,and this merge request extend it to :read a file from data dir and return it as string I guess the original get_data should be named load_json_data:)

Given that, I'd do it in #286 https://github.com/neoave/mrack/pull/286

sure,I can add it to #286, it's just that I'm always told to separate different things in several merge request,as I tend to send merge requests containing lots of files:) I think the new get_data function could be used by other testcases,not only the one I scripted,so I separated it.

On Mon, Feb 5, 2024 at 9:04 PM Petr Vobornik @.***> wrote:

@.**** requested changes on this pull request.

I don't think this change is improving the code base.

The get_data purpose was to read a file from data dir and return it as string. Now this is extending it to return it as dict parsed from json when the file ends with json. This is hard to figure out from the method name.

Note: retrospectively, the get_data name is not the best.

It would be better to have a separate method for this with a name that reflect the new behavior. E.g. load_json_data.

About commit message type: this would not be "feat" as it is not adding any feature.

Given that, I'd do it in #286 https://github.com/neoave/mrack/pull/286

  • as a part of a commit that is delivering test. There is no point in adding a helper function / helper function behavior separately when it is not being used.

— Reply to this email directly, view it on GitHub https://github.com/neoave/mrack/pull/287#pullrequestreview-1862723849, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23FKQ2UHQ24TSJJW553YSDKFZAVCNFSM6AAAAABCY2EY52VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNRSG4ZDGOBUHE . You are receiving this because you authored the thread.Message ID: @.***>

skycastlelily commented 7 months ago

@pvoborni Done:)

pvoborni commented 7 months ago

The original get_data's function is: return dict parsed from json file,and this merge request extend it to :read a file from data dir and return it as string I guess the original get_data should be named load_json_data:)

Yeah, you are right. And it would be good to rename the original.

But the main point is still valid - to not mix those in single function.

skycastlelily commented 7 months ago

But the main point is still valid - to not mix those in single function.

Make sense,and updated:)

pvoborni commented 7 months ago

Closing as similar thing is done in https://github.com/neoave/mrack/pull/286