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

CI: Update labeler’s labels #3349

Merged
merged 5 commits into from
Jan 11, 2024
Merged

Conversation

echoix
Copy link
Member

@echoix echoix commented Jan 9, 2024

As promised, a little follow-up on the labeler’s labels.

Was missing imagery and raster3d.
Added patterns to match for files of a module inside a the lib folder.

Excluded RFC to not match Markdown or docs.

One of my goals was to have the docs label match when it is a docs-only PR, that could be quicker to review

@echoix echoix requested a review from nilason January 9, 2024 04:03
@github-actions github-actions bot added the CI Continuous integration label Jan 9, 2024
Co-authored-by: Markus Neteler <[email protected]>
@echoix
Copy link
Member Author

echoix commented Jan 9, 2024

If it's correct, add the approval and set the automerge and done!

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

How about to map the directories in scripts based on the prefix?

@echoix
Copy link
Member Author

echoix commented Jan 9, 2024

I see them now. What is the difference between those in scripts rather than in their respective folders?

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

I see them now. What is the difference between those in scripts rather than in their respective folders?

Python based.

@echoix
Copy link
Member Author

echoix commented Jan 9, 2024

And vs in the python/grass/(module type name)/ folders? There's something by about imaging and temporal there

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

And vs in the python/grass/(module type name)/ folders? There's something by about imaging and temporal there

I would leave that out.

@echoix
Copy link
Member Author

echoix commented Jan 9, 2024

And vs in the python/grass/(module type name)/ folders? There's something by about imaging and temporal there

I would leave that out.

I was trying to understand the fine-details of the project's folder structure, to know if the script folder might be superfluous, if some of the code is also tucked inside the python/grass/ folder.

@marisn
Copy link
Contributor

marisn commented Jan 9, 2024

I was trying to understand the fine-details of the project's folder structure, to know if the script folder might be superfluous, if some of the code is also tucked inside the python/grass/ folder.

I think you meant gui/scripts and gui/wxpython for modules. In the past scripts/ contained various non-C modules (mainly shell scripts). Nowadays Python has taken everything over, but they still are ordinary, independent modules, that can run without gui. gui/ contains some modules with tight integration into gui. Those would not make much sense without gui. python/ on the other hand is a python equivalent of lib/. As modules are not designed to be loaded directly into python but just executed as standalone processes, such division makes sense.
Overall GRASS has fat modules and slim library. It doesn't help code re-usability but is much easier to write.

Copy link
Member

@neteler neteler left a comment

Choose a reason for hiding this comment

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

Good improvement. We may revisit and fine-tune it in future after some time, as needed.

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

Looks good to me, good to go!

In case you want to fine-tune, the following may be added:

display:
  - changed-files:
      - any-glob-to-any-file:
          - display/**/*
general:
  - changed-files:
      - any-glob-to-any-file:
          - general/**/*
misc:
  - changed-files:
      - any-glob-to-any-file:
          - misc/**/*

and map the following:

  • scripts/db.* -> database
  • scripts/d.* -> display
  • scripts/g.* -> general
  • scripts/i.* -> imagery
  • scripts/m.* -> misc
  • scripts/r.* -> raster
  • scripts/r3.* -> raster3d
  • scripts/v.* -> vector

@echoix
Copy link
Member Author

echoix commented Jan 9, 2024

On a second thought, since I think the use cases for the labels could be either to help finding issues that matches a reviewer's expertises, or to eventually help out with releasing/release notes, or simply to give a high level overview of what is touched, I removed the Markdown section.

It is already redundant with the docs label, and it doesn't really bring a value to discriminate between the expertise needed. If you see a PR with only docs as a label, it's a docs only change, if it is docs and modules and a category, the PR updates the documentation of a module.

I didn't remove yet the HTML, as I didn't search yet if there is a template (like for docs) that might necessitate a web expertise. Same thing for CSS and JavaScript, these are different expertises.

@nilason nilason requested a review from neteler January 10, 2024 14:18
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

(The "binder" and "pre-commit" parts are questionable, but CI is close enough).

@echoix
Copy link
Member Author

echoix commented Jan 10, 2024

Ya, but I wasn't able to find a better place, and it isn't "build" neither. So I falled back to "who would review this?", and it's about the same as the other code quality and building infrastructure as CI.

@echoix echoix merged commit f7650a9 into OSGeo:main Jan 11, 2024
24 checks passed
@echoix echoix deleted the labeler-subdirectories branch January 11, 2024 12:56
@neteler neteler added this to the 8.4.0 milestone Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants