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

Add a KBI with insights from #15 on config overrides #98

Merged
merged 3 commits into from Jul 25, 2023
Merged

Conversation

adswa
Copy link
Contributor

@adswa adswa commented Jun 30, 2023

This KBI is an attempt to document some of the knowledge gathered in #15, and point out that shortcomings of datalad configs in core can be fixed by using next. It is still a bit vague for my liking, but I struggled with finding a better way - maybe this can be shaped further in the next user support session.

It addresses parts of #15.

Copy link
Contributor

@christian-monch christian-monch left a comment

Choose a reason for hiding this comment

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

Thx @adswa . There are a few small things to address, once they are done it should be good to merge FMPOV

kbi/0026/index.rst Outdated Show resolved Hide resolved
kbi/0026/index.rst Outdated Show resolved Hide resolved
Comment on lines 35 to 36
This shortcoming was `fixed`_ in May 2023.
At the time of writing, and the fix can be enabled with an installation of `datalad-next`_ and the configuration ``datalad.extensions.load next``, as detailed in the projects documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that this was meant:

Suggested change
This shortcoming was `fixed`_ in May 2023.
At the time of writing, and the fix can be enabled with an installation of `datalad-next`_ and the configuration ``datalad.extensions.load next``, as detailed in the projects documentation.
This shortcoming was `fixed`_ in May 2023. The fix can be enabled with an installation of `datalad-next`_ and the configuration ``datalad.extensions.load next``, as detailed in the projects documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean, one sentence per line?

Copy link
Contributor

Choose a reason for hiding this comment

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

I should have more clearly stated my thoughts!

I assumed that line 35 and line 36 were split by "edit-error" and meant to be one line, that expresses what I suggested as line 35, i.e. "This shortcoming was fixed_ in May 2023. The fix can be enabled with an installation of datalad-next_ and the configuration datalad.extensions.load next, as detailed in the projects documentation.".

But I could not be certain that was what you intended, so I commented with: "Not sure that this was meant"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typically I write text as one sentence per line, because it makes it easier to see which logical unit changes belong to with a Git diff.
It does not make any difference for the rendering (see e.g., the preview from the CI https://psyinf-knowledge-base--98.org.readthedocs.build/kbi/0026/index.html) in case this is what you are concerned with? Only if a blank line is in between two sentences the paragraph is split.
image

kbi/0026/index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@mslw mslw left a comment

Choose a reason for hiding this comment

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

Nice and clear to me; once the last small thing is addressed, should be merged.

kbi/0026/index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@jsheunis jsheunis left a comment

Choose a reason for hiding this comment

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

Definitely a necessary KBI, thanks 👍

Made a suggestion and a comment.

kbi/0026/index.rst Outdated Show resolved Hide resolved
This shortcoming was `fixed`_ in May 2023.
At the time of writing, the fix can be enabled with an installation of `datalad-next`_ and the configuration ``datalad.extensions.load next``, as detailed in the projects documentation.

The alternative workaround requires the use of Git configurations in environment variables.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not obvious to me how this line connects the following paragraph to the previous one. To what specific issue is the use of Git configurations in environment variables an "alternative workaround"? I read it as "the following is an alternative to using datalad-next to solve the issue of not being able to override the git user name", but what then follows is a description and solution of a different issue (cloning from a shared repo). I'm guessing the issues and workarounds are supposed to be interpreted on a more abstract level, and examples are given to make it more tangible, but I think some rephrasing could be done to make this message clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the critical review, I really appreciate that! I have pushed a restructuring, could you give it a read when its convenient for you and see if that improves coherence?

Copy link
Contributor

Choose a reason for hiding this comment

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

Restructured content makes it all much clearer, thanks!

kbi/0026/index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@loj loj left a comment

Choose a reason for hiding this comment

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

This is a helpful KBI! Thanks @adswa!

@jsheunis are there more changes you wanted or can this be merged?

@jsheunis
Copy link
Contributor

Nope, I'm happy with the adjustments.

@adswa adswa merged commit a0f5637 into main Jul 25, 2023
3 checks passed
@adswa adswa deleted the config-overrides branch July 25, 2023 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants