019 minor fixes #292

Evildoor · 2019-10-28T12:13:28Z

Fix various minor problems with the stage.

Add/improve help messages and options to use them properly.
Add message when a default config is used.
Add comments describing script logic to esFormat.php.
Fix errors discovered during work on the PR (eb7759a, ae23121).
Correct typos, lack of numeration in README, and other minor readability issues.

https://trello.com/c/ZUvvKjKC
https://trello.com/c/wjTOlVWt

mgolosova

All comments are below, but this one just is not related to any specific piece of code: since this PR adds descriptions to almost all of the PHP functions, would be nice to have one for getAction() as well.

Most of the comments are just suggestions: current version of the PR already makes things better than they were so it makes sence to merge it even as it is right now. So please let me know if you would prefer to leave it as it is or will make some changes.

Thank you for bringing some order to the Chaos! :)

Utils/Dataflow/019_esFormat/esFormat.php

Utils/Dataflow/019_esFormat/run.sh

Evildoor · 2019-12-27T14:09:31Z

would be nice to have description for getAction() as well

Done.

mgolosova

Few more comments/suggestions.

Utils/Dataflow/019_esFormat/esFormat.php

Utils/Dataflow/019_esFormat/run.sh

mgolosova · 2020-02-10T09:03:30Z

Stage 019: мелкие исправления

Utils/Dataflow/019_esFormat/esFormat.php

Evildoor · 2020-03-23T11:28:59Z

Rebased the branch on master to remove the commit that have already been merged separately (#322).

mgolosova

I am sorry I missed a couple of things during previous review.
I am asking you to change the following things:

return value of run.sh when --help option is used;
fix description of constructDataJson() (the part related to the "doc_as_upsert");
mark -- as required delimiter between arguments of run.sh and esFormat.php.

And, optionally -- add the link to the ES documentation to the constructActionJson function's description.

Utils/Dataflow/019_esFormat/run.sh

Utils/Dataflow/019_esFormat/esFormat.php

Utils/Dataflow/019_esFormat/run.sh

- Separate the definitions into default values (constants) and variables. - Add descriptions.

Exit shouldn't depend on usage()'s return value.

run.sh is a wrapper around PHP script and even uses it to explain the meaning of run.sh's ARGS.

Bring the style closer to that of pyDKB/dataflow/stage/AbstractStage.py.

Having whole message on the same level of indent makes it easier to read and edit.

Details about passing arguments to esFormat.php and getting help for it are unnecessary when the script is called from run.sh.

- Reword the description: 'Elasticsearch config' can be understood in different ways. - Display the default value of the config. - Move the definition of two variables before the usage(): previous point requires one of them.

It is not really important whether a document will be processed alone or in bulk.

The variable's value gets overwritten later in code, so it shouldn't be initialized here to avoid confusion.

It is not changed through the script, so it's actually a 'constant'.

To avoid confusion with DEFAULT_ACTION being changed during the script execution, a new variable is added, while DEFAULT_ACTION's value remains constant and is used, if necessary, to define the new variable. Remove the lengthy comment about DEFAULT_ACTION as it is no longer necessary.

This separator is used in many bash commands - while its role may be obvious to some, others may be unaware of it.

mgolosova

That's the last change request here, this time's (almost) for sure.

Commit message of the fe29851 ilooks very confusing: 0 is rarely referred to as a error status, so when I see error status 0 I stumble for a moment and not sure what to think about. Exit status, satus code, exit code, return code, maybe exit value or something?

If you don't mind, please change this commit message -- and let me know what's your decision concerning the getAction() description.

mgolosova · 2020-04-13T11:30:04Z

Utils/Dataflow/019_esFormat/esFormat.php

+  Action is 'update' if {'_update': true} key-value pair is 
+  present in the document. Otherwise, default value is returned.


I'm afraid that after 67a80d8 (#320) this description is incomplete.
I can merge this PR to immediately fix it in the next one, or you can merge master here and update the description accordingly.

Done, 3a078d4.

Non-zero exit status usually signals that some kind of error occured. Co-Authored-By: Marina Golosova <golosova.marina@gmail.com>

-- is supposed to separate arguments for run.sh and PHP script. The fact that everything works without it is, actually, a bug that would be dealt with later.

Evildoor · 2020-04-13T17:30:16Z

Comment about "error status" is valid, I probably made a mistake while typing. Fixed.

mgolosova · 2020-04-14T10:58:04Z

@Evildoor, thank you!
Approved and merged now. If you do not need this branch for the further work, please remove it.

Evildoor self-assigned this Oct 28, 2019

Evildoor changed the title ~~[WIP] 019 minor fixes~~ 019 minor fixes Nov 21, 2019

Evildoor requested a review from mgolosova November 21, 2019 13:17

mgolosova reviewed Nov 26, 2019

View reviewed changes

Evildoor force-pushed the 019-minor-fixes branch 2 times, most recently from 9e9a2dc to 7b16c01 Compare December 27, 2019 13:52

mgolosova reviewed Jan 14, 2020

View reviewed changes

Utils/Dataflow/019_esFormat/esFormat.php Outdated Show resolved Hide resolved

Utils/Dataflow/019_esFormat/esFormat.php Outdated Show resolved Hide resolved

Utils/Dataflow/019_esFormat/run.sh Show resolved Hide resolved

Utils/Dataflow/019_esFormat/run.sh Outdated Show resolved Hide resolved

mgolosova reviewed Jan 14, 2020

View reviewed changes

Utils/Dataflow/019_esFormat/run.sh Outdated Show resolved Hide resolved

Evildoor force-pushed the 019-minor-fixes branch from aad01ab to 5cf279d Compare January 24, 2020 10:43

mgolosova mentioned this pull request Feb 17, 2020

DF/019: Fix error with update and index in same action. #322

Merged

mgolosova reviewed Feb 20, 2020

View reviewed changes

Utils/Dataflow/019_esFormat/esFormat.php Show resolved Hide resolved

Evildoor added 8 commits March 23, 2020 12:07

Fix a typo.

de8d014

State what config is expected

3568321

Add an option to get help with --help/-h keys.

685c710

Add a message for default config being used.

e19300f

Add missing numeration to README.

d11b45b

Add description for check_input().

7b5f0f6

Add description for convertIndexToLowerCase().

f893b6a

Add description for constructActionJson().

9e921cb

Evildoor force-pushed the 019-minor-fixes branch from 5cf279d to 9513771 Compare March 23, 2020 11:11

mgolosova requested changes Mar 25, 2020

View reviewed changes

Evildoor force-pushed the 019-minor-fixes branch from 7812ed6 to 21e81e2 Compare April 7, 2020 16:48

Evildoor added 5 commits April 11, 2020 14:36

Add description for constructDataJson().

3b22230

Update readability for list of definitions.

3327c73

- Separate the definitions into default values (constants) and variables. - Add descriptions.

Add comments to actions in main logic.

4aa44ec

Add help message and options to show it.

2bfbf7c

Always exit after usage().

c6977b4

Exit shouldn't depend on usage()'s return value.

Evildoor added 15 commits April 11, 2020 14:36

Call esFormat.php's help from run.sh's usage().

345d441

run.sh is a wrapper around PHP script and even uses it to explain the meaning of run.sh's ARGS.

Update run.sh's usage() style.

d6e3734

Bring the style closer to that of pyDKB/dataflow/stage/AbstractStage.py.

Move start of message to separate line.

808fbb1

Having whole message on the same level of indent makes it easier to read and edit.

Add info about usage of stdin to help message.

8d253b9

Shorten run.sh's help.

002d24f

Details about passing arguments to esFormat.php and getting help for it are unnecessary when the script is called from run.sh.

Clarify the config description.

05303e5

- Reword the description: 'Elasticsearch config' can be understood in different ways. - Display the default value of the config. - Move the definition of two variables before the usage(): previous point requires one of them.

Simplify the description.

0048d97

It is not really important whether a document will be processed alone or in bulk.

Add description for getAction().

bb2deb7

Remove index initialization with NULL.

2963b3b

The variable's value gets overwritten later in code, so it shouldn't be initialized here to avoid confusion.

Move UPDATE_RETRIES definition.

339a687

It is not changed through the script, so it's actually a 'constant'.

Expand DEFAULT_ACTION description.

68110d1

Add a note about help messages' dependance.

7d3ed8b

Add explanation for '--' argument.

71b0b39

This separator is used in many bash commands - while its role may be obvious to some, others may be unaware of it.

Add link to ES docs.

651b5e9

Evildoor force-pushed the 019-minor-fixes branch from 21e81e2 to fe29851 Compare April 11, 2020 12:38

mgolosova requested changes Apr 13, 2020

View reviewed changes

Evildoor and others added 5 commits April 13, 2020 17:29

Change exit status to 0.

2dde924

Non-zero exit status usually signals that some kind of error occured. Co-Authored-By: Marina Golosova <golosova.marina@gmail.com>

Remove unnecessary brackets.

76dfa03

Update optionality of arguments.

90b5003

-- is supposed to separate arguments for run.sh and PHP script. The fact that everything works without it is, actually, a bug that would be dealt with later.

Merge branch 'master' into 019-minor-fixes

0d91916

Adapt stage to incomplete data handling changes.

3a078d4

Evildoor force-pushed the 019-minor-fixes branch from a1eb61b to 3a078d4 Compare April 13, 2020 17:28

mgolosova approved these changes Apr 14, 2020

View reviewed changes

mgolosova merged commit b3e551d into master Apr 14, 2020

Evildoor deleted the 019-minor-fixes branch April 14, 2020 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

019 minor fixes #292

019 minor fixes #292

Evildoor commented Oct 28, 2019 •

edited

Loading

mgolosova left a comment •

edited

Loading

Evildoor commented Dec 27, 2019 •

edited

Loading

mgolosova left a comment

mgolosova commented Feb 10, 2020

Evildoor commented Mar 23, 2020 •

edited

Loading

mgolosova left a comment

mgolosova left a comment

mgolosova Apr 13, 2020

Evildoor Apr 13, 2020

Evildoor commented Apr 13, 2020

mgolosova commented Apr 14, 2020

		Action is 'update' if {'_update': true} key-value pair is
		present in the document. Otherwise, default value is returned.

019 minor fixes #292

019 minor fixes #292

Conversation

Evildoor commented Oct 28, 2019 • edited Loading

mgolosova left a comment • edited Loading

Choose a reason for hiding this comment

Evildoor commented Dec 27, 2019 • edited Loading

mgolosova left a comment

Choose a reason for hiding this comment

mgolosova commented Feb 10, 2020

Evildoor commented Mar 23, 2020 • edited Loading

mgolosova left a comment

Choose a reason for hiding this comment

mgolosova left a comment

Choose a reason for hiding this comment

mgolosova Apr 13, 2020

Choose a reason for hiding this comment

Evildoor Apr 13, 2020

Choose a reason for hiding this comment

Evildoor commented Apr 13, 2020

mgolosova commented Apr 14, 2020

Evildoor commented Oct 28, 2019 •

edited

Loading

mgolosova left a comment •

edited

Loading

Evildoor commented Dec 27, 2019 •

edited

Loading

Evildoor commented Mar 23, 2020 •

edited

Loading