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

Spelling fixes in raster addons documentation #664

Merged
merged 48 commits into from
Jan 12, 2022

Conversation

echoix
Copy link
Member

@echoix echoix commented Jan 8, 2022

Following some work already pulled recently, I continued proofreading the documentation and strings from the documentation, staying in the raster modules. Since I've stopped working on it without finishing the 167 subfolders (41 folders done on 167), I think it is still valuable to fine-tune and pull these changes, since I still passed through 20k lines of code.

All modules from r.accumulate to r.futures (alphabetically), inclusively, were proofread and changed if needed, except r.estimap.recreation and r.damflood. I skipped r.estimap.recreation since it appears to be developed on an external GitLab repository, so fetching upstream changes are more appropriate. For r.damflood, the changes are bigger and more complex, they are in another branch and the pull request will follow.

I saw that the r.edm.eval add-on is only a shell script, that seems to be what add-ons were in GRASS 6 if I understood correctly. Does that mean that it is incompatible with GRASS 7 or GRASS 8?

I have some other notes that I took on a paper when working on this, but unfortunately I don't have the paper with me at the moment, so I will add the revision comments to the specific lines where I wasn't sure either tomorrow or later, anyway this PR should take a couple days to review.

@echoix
Copy link
Member Author

echoix commented Jan 9, 2022

+1 for the spell checks test in the CI. For example, MegaLinter has cspell and misspell.

Indeed, I checked it up, MegaLinter seems a good upgrade to the project. It seems that MegaLinter was based off a rejected PR of Super-Linter, which is the GitHub general linter used here. So, that would mean replacing this, plus the Black and flake8 actions, since they are already included?

I'm thinking of using it locally for the following checks, if possible. However, many .c files were not formatted consistently at all, so it might need a separate PR. I remember fighting my VScode to not include the "fixed" indentation when posting back the fixes, since some mixed tab and spaces (in the same line) were used. I wanted to keep the diffs clean.

@ecodiv
Copy link
Contributor

ecodiv commented Jan 9, 2022

Great work! I quickly went through some of the commits, but I am not sure how the review process works. One can review and comment or approve each commit separately? If I open a commit, there is this green button 'Review changes'. Is that button about the commit I opened, or the whole list?

@ecodiv
Copy link
Contributor

ecodiv commented Jan 9, 2022

I saw that the r.edm.eval add-on is only a shell script, that seems to be what add-ons were in GRASS 6 if I understood correctly. Does that mean that it is incompatible with GRASS 7 or GRASS 8?

That is one I still wanted to port to Python, but to be honest sort of forgot about. It is mostly R code, should not be too difficult to change to Python. I'll have a look at it.

@neteler
Copy link
Member

neteler commented Jan 9, 2022

One can review and comment or approve each commit separately?

Yes. You can comment on each line or propose edits:

image

Edit: pull down the blue "+" to edit multiple lines at once!

@neteler
Copy link
Member

neteler commented Jan 9, 2022

Following some work already pulled recently, I continued proofreading the documentation and strings from the documentation

Thanks for your great work.

BTW: in GRASS-core, we also have the script codespell.py borrowed from GDAL, see https://github.com/OSGeo/grass/blob/main/utils/fix_typos.sh (I used it once, time ago).

@neteler
Copy link
Member

neteler commented Jan 9, 2022

However, many .c files were not formatted consistently at all, so it might need a separate PR.

For this, we have

They are naturally pretty invasive, so some care needs to be taken with the open large pull requests (maybe skip indentation of those for now till they have been merged).

@echoix
Copy link
Member Author

echoix commented Jan 9, 2022

They are naturally pretty invasive, so some care needs to be taken with the open large pull requests (maybe skip indentation of those for now till they have been merged).

Of course. I was thinking of maybe having a preparatory pull request if we were to have a CI that would make sure it doesn't get dirty again.

@echoix
Copy link
Member Author

echoix commented Jan 9, 2022

Ok, for my notes that I took, finally there isn't much more than the sentence in r.edm.eval, and the modules that I skipped.
The remaining fix was a spelling fix in the r.extract.py append_random() that should be fixed upstream since it was copied from the core repo, and a reminder to enter an issue to flag that r.diversity might not work in grass 8 since the code explicitly makes extra checks and builds a path with "GRASS7" in its main().

So once everyone who wants to review has reviewed, it will be ready to merge.

@echoix echoix marked this pull request as ready for review January 9, 2022 17:56
@echoix
Copy link
Member Author

echoix commented Jan 9, 2022

Great work! I quickly went through some of the commits, but I am not sure how the review process works. One can review and comment or approve each commit separately? If I open a commit, there is this green button 'Review changes'. Is that button about the commit I opened, or the whole list?

@ecodiv You can open the commits or the files, and press the "+" like @neteler showed. A comment or suggested change can be made for the cumulative code changed in the PR (like if additionnal commits change something and it is still not right). See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request if you want to learn more. :)

Copy link
Member Author

@echoix echoix left a comment

Choose a reason for hiding this comment

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

I found unnecessary lines changed in a diff.

src/raster/r.edm.eval/r.edm.eval.html Outdated Show resolved Hide resolved
src/raster/r.fill.category/r.fill.category.html Outdated Show resolved Hide resolved
src/raster/r.findtheriver/r.findtheriver.html Outdated Show resolved Hide resolved
src/raster/r.flexure/r.flexure.py Outdated Show resolved Hide resolved
src/raster/r.flexure/r.flexure.html Outdated Show resolved Hide resolved
Remove inserted lines from diff
src/raster/r.droka/r.droka.py Outdated Show resolved Hide resolved
src/raster/r.droka/r.droka.py Outdated Show resolved Hide resolved
@neteler
Copy link
Member

neteler commented Jan 9, 2022

a reminder to enter an issue to flag that r.diversity might not work in grass 8 since the code explicitly makes extra checks and builds a path with "GRASS7" in its main().

(hopefully addressed in #667)

@echoix
Copy link
Member Author

echoix commented Jan 11, 2022

After resolving the last two opened conversations, I think the PR can be merged.

@wenzeslaus
Copy link
Member

Sorry, unrelated to this already long PR discussion, but anyway:

+1 for the spell checks test in the CI. For example, MegaLinter has cspell and misspell.

So, that would mean replacing [Super-Linter]...

Maybe, maybe not, my guess is that MegaLinter and Super-Linter diverged now enough that each has its own merits.

...plus the Black and flake8 actions, since they are already included?

Not necessarily, direct and possibly finer control over Flake8 and Black (and possibly Pylint) is likely desired, e.g., due to changes in Black versions.

@neteler neteler changed the title Spelling fixes in raster plugins documentation Spelling fixes in raster addons documentation Jan 12, 2022
@neteler
Copy link
Member

neteler commented Jan 12, 2022

If there are no objections, I'll merge this PR. Thanks for your efforts @echoix!

@echoix
Copy link
Member Author

echoix commented Jan 12, 2022

Looks good to me! I will be ready to continue next weekend!

@neteler neteler merged commit 11cdf2d into OSGeo:grass8 Jan 12, 2022
@echoix
Copy link
Member Author

echoix commented Jan 31, 2022

+1 for the spell checks test in the CI. For example, MegaLinter has cspell and misspell.

Jusr to let you know that its been two weekends I'm playing around with MegaLinter in my fork. I'm learning how it works and configuring it correctly to be confident about it when it will be time to make a PR. Last week, the maintainer of MegaLinter jumped into my test PR, and made some performance changes to MegaLinter itself with what I observed. He even forked my fork to test an even more optimal configuration. I'm writing it here just to not make some duplicated work if someone though of implementing it while I'm working on it. My goal is to try it out, have it here, and one day have it in the core repo too.

IvanMarchesini pushed a commit to IvanMarchesini/grass-addons that referenced this pull request Feb 24, 2022
* Spelling fixes in r.accumulate
* Spelling fixes in r.flexure
* Spelling fixes in r.agent
* Fix r.connectivity page description truncated
* Spelling fixes r.area
* Spelling fixes in r.area.createweight. Also, some links had some spaces shown in the rendered page, since the line was wrapped in the middle of the URL. It is now fixed.
* Spelling fixes in r.connectivity.corridors
* Spelling fixes in r.basin
* Spelling fixes for r.bearing.distance
* Spelling fixes for r.bioclim
* Spelling fixes in r.bitpattern. And necessary indentation fixes to the touched header, to be consistent.
* Spelling fixes in r.catchment
* Spelling fixes in r.category.trim
* Spelling fixes for r.change.info
* Spelling fixes for r.clip
* Spelling fixes for r.colors.cubehelix
* Spelling fixes for r.colors.matplotlib
* Spelling fixes for r.colors.out_sld
* Spelling fixes in r.confusionmatrix
* Spelling fixes in r.cpt2grass
* Spelling fix in r.crater
* Spelling fixes in r.denoise
* r.diversity: HTML formating of parameter in manual
* r.droka: Spelling fixes
* r.droka: Spelling fixes and some english comments
* r.edm.eval: Spelling fixes
* r.edm.eval: HTML formating of parameters
* r.euro.ecosystem: Spelling fixes
* r.exdet: Spelling fixes
* r.fidimo: Spelling fixes
* r.fill.category: Spelling fixes
* r.findtheriver: Spelling fixes
* r.flexure: Spelling fixes
* r.flowfill: Spelling fixes
* r.forestfrag: Spelling fixes
* r.futures.demand: Spelling fixes
* r.futures.devpressure: Spelling fixes
* r.futures.parallelpga: Spelling fixes
* r.futures.pga: Spelling fixes
* r.futures.potential: Spelling fixes
* r.futures.potential: Spelling fixes
* r.futures.potsurface: Spelling fixes
* r.futures: Comment old SVN date
* r.futures.calib: Spelling fixes
* r.futures.demand: Additional spelling fixes
* r.droka: Apply translation suggestions
@echoix echoix deleted the Spelling-patch-2 branch December 30, 2023 14:03
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.

6 participants