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

Doc: flake8 todos #1981

Merged
merged 3 commits into from
Jan 2, 2025
Merged

Doc: flake8 todos #1981

merged 3 commits into from
Jan 2, 2025

Conversation

moe-ad
Copy link
Contributor

@moe-ad moe-ad commented Jan 2, 2025

Related to #296. flake8-todos ruff rule is enabled via this PR

@moe-ad moe-ad self-assigned this Jan 2, 2025
@moe-ad moe-ad requested a review from RobPasMue January 2, 2025 10:59
@RobPasMue
Copy link
Member

Hi @moe-ad! Could we separate this PR in two and have the license headers updated in a different PR? Otherwise it makes it difficult to review...

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (fd0c755) to head (b37b4bb).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1981      +/-   ##
==========================================
- Coverage   88.48%   88.39%   -0.10%     
==========================================
  Files          89       89              
  Lines       10251    10251              
==========================================
- Hits         9071     9061      -10     
- Misses       1180     1190      +10     

@RobPasMue
Copy link
Member

RobPasMue commented Jan 2, 2025

Also, the TODO URL should point to an issue where we are keeping track of that request. We shouldn't be pointing to a PR URL. See ansys/pyansys-geometry#1319 as an example. The first time I implemented the flake8-todo task, I created this longterm tracker. Ideally, each todo should have an independent issue though.

@moe-ad
Copy link
Contributor Author

moe-ad commented Jan 2, 2025

@RobPasMue, thanks for the review. I have moved the license header update to another PR as requested.

Concerning the issue vs PR links, the most recent of the TODOs dates back to 2022, while others were added in 2021. I searched for related issues but found none. I decided to add PR links instead for context because of the following reasons:

  • Some TODOs will involve some server side wrapping, implying significant work/decision making by the development team concerning whether/not they want to proceed (which they haven't for years now of course). So can we just create an issue regardless, even if it may never be addressed eventually?
  • For those that seem to be pure python tasks, is it ok to create issues without providing much detail about exactly what needs to be done? Including enough detail will properly require inputs from PyDPF developers, and most are on leave except maybe @cbellot000.

What do you advise? Or maybe I should just follow your footsteps in given example i.e. just create one issue and itemize those TODOs.

@RobPasMue
Copy link
Member

RobPasMue commented Jan 2, 2025

IMO the only way you will actually keep track of pending work is through issues, not PRs. It's hard to keep track of them otherwise. I'd advise to create a longterm tracker and let the @ansys/pydpf-admins dev team handle them whenever they have the chance.

@moe-ad
Copy link
Contributor Author

moe-ad commented Jan 2, 2025

@RobPasMue thanks!
I have implemented your suggestion.

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM!

@moe-ad moe-ad marked this pull request as ready for review January 2, 2025 13:59
@moe-ad moe-ad merged commit b7f89f7 into master Jan 2, 2025
63 of 64 checks passed
@moe-ad moe-ad deleted the doc/flake8-todos branch January 2, 2025 14:39
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.

2 participants