-
Notifications
You must be signed in to change notification settings - Fork 122
tutorial-improvements #1311
tutorial-improvements #1311
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, some changes are very nice! A lot of clutter was removed!
@@ -185,12 +180,12 @@ Thus we are interested in the value we use: | |||
char *val = keyString(k); | |||
``` | |||
|
|||
Finally, we need to convert the configuration value to the datatype we | |||
Now we need to convert the configuration value to the data-type we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange recommendations: "now" is not a good style, "finally" is not necessary but okay.
data-type also seems very non standard: it seems to be "data type" see #920
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't display recommendations, it only complains that finally isn't really necessary. but you are right now isn't really necessary either imo its perfectly fine to start with We.
fixed data type.
@@ -205,39 +200,39 @@ For more information about that, continue reading | |||
|
|||
## Specification ## | |||
|
|||
Now, we have a fully working configuration system without any hard-coded | |||
information (such as configuration files). So we already gained something. | |||
Now, we have a fully working configuration system without any hardcoded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All forms seem to be acceptable: https://en.wikipedia.org/wiki/Hard_coding
Why change it?
Does the tool allow automatic transformation so that we have it easier in future when we change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so we should still decide for a common style for our usage and not mix it up. a quick grep indicates that apart from some news that probably don't matter hardcode or its variations are only used in the application integration tutorial, so its hard to find a majority to follow here.
The tool is a linter, so all it does is complaining :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should write a script that automatically does the replacements to a canonical form. Discussions for that in #920
This technique provides complete transparency how a program will fetch a configuration | ||
value. In practice that means that: | ||
This technique provides complete transparency how a program will fetch a | ||
configuration value. In practice that means that: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please separate formatting changes with other changes in future. It is difficult to read this PR.
(At least different commits!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, will do in future commits
world it is only a matter of time until other software also wants to | ||
access the configuration, and with elektrified software it is possible | ||
format and semantics of the configuration. In the interconnected | ||
world it's a matter of time until other software also wants to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"it is" is preferable in more formal English. For tutorials both are okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess thats a matter of preference. i think it's is a bit easier to read and fine to use unless writing a paper. But grep reveiled that it is is much more common throughout our documents than it's though, so ill stick to that.
Thank you! Is this ready to merge? The flag shows still "work in progress"? Its better if we keep PRs small and merge them soon. |
i'm currently checking and rating the other tutorials too, but i can do a separate PR for each of them if i do some changes. In that case, it is ready to merge. |
I always used the unix tool |
Thank you! That is a nice PR! Looking forward to improvements in the other tutorials! |
Purpose
In the course of #1176 i've checked the text via a linter (https://github.com/btford/write-good) and saw that there is probably some room for improvement regarding words which don't really add much value to a sentence like only or using too much passive form. Therefore i reworked some parts which i think can be improved.
Checklist
@markus2330 do you think this way we make the tutorials easier to read? If you do I will also try to improve the other tutorials as well.