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 definition of “co-located grids” to the glossary #139

Merged
merged 4 commits into from
Oct 21, 2022
Merged

Add definition of “co-located grids” to the glossary #139

merged 4 commits into from
Oct 21, 2022

Conversation

MGomezN
Copy link
Member

@MGomezN MGomezN commented Sep 23, 2022

Addition requested in #138

Relevant issues/PRs:

Fixes #138

Addition requested in #138
Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @MGomezN ❤️

I have a small suggestion for rewording and linking this back to the tutorial.

doc/glossary.rst Outdated Show resolved Hide resolved
Co-authored-by: Leonardo Uieda <leouieda@gmail.com>
Substitute the occurrence of "co-located grids" with :term:`co-located grids` to automatically link to this glossary entry.
@leouieda
Copy link
Member

Pulled this commit into #139 since they make more sense as a single change.

@leouieda leouieda closed this Oct 21, 2022
@leouieda leouieda reopened this Oct 21, 2022
@leouieda
Copy link
Member

Oops, just noticed that I posted messages to the wrong PRs...

@leouieda
Copy link
Member

@MGomezN this PR and #140 are better as a single change (adding to the glossary and then using the glossary term in the docs). So I pulled the commit from that PR into this one. So I'll be closing that one and commenting here instead.

In the future, you can keep pushing to the same branch to update the PR if there are further changes you want to make before the PR is merged. The balance of what goes into a single PR and what should be multiple is tricky and we've been playing it by ear. A good rule of thumb is: does this PR make sense on its own? If not, then maybe further changes can be added there. Otherwise, then it would be better as a follow-up PR.

None of this is too strict and it's always possible to change as we go along (pulling commits into separate PRs is doable with a little git magic 😉). So don't worry too much about getting it right from the start. We rarely do and often have discussions along these lines.

Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

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

@MGomezN left a few suggestions which I'll merge into this PR so we can get the PR merged as well 🙂 Sorry for the delay on this.

doc/glossary.rst Outdated Show resolved Hide resolved
doc/overview.rst Outdated Show resolved Hide resolved
@leouieda leouieda changed the title Add co-located to the glossary. Add definition of “co-located grids” to the glossary Oct 21, 2022
@leouieda leouieda merged commit 76c7836 into fatiando:main Oct 21, 2022
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.

Add meaning of "co-located"
2 participants