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 skip status to ignore selected posts #3305

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

GiovanH
Copy link
Contributor

@GiovanH GiovanH commented May 19, 2024

Pull Request Checklist

Resolves: #3304

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools
  • Added tests for changed code
  • Updated documentation for changed code

Proof of concept, let me know if you like this direction and I'll write test cases and documentation.

@GiovanH
Copy link
Contributor Author

GiovanH commented May 19, 2024

Because I have many drafts, this also has the side effect of speeding up my publish builds by 80%. That's nice.

Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

Nice work, @GiovanH. I am not sure what the best name for this status should be, but I think I would prefer skip over disabled. (I also considered ignore, but I think skip is clearer.)

Also, since there are a number of new commits to main, this PR should be rebased to pick up those changes.

I realize this is still a draft PoC, but in the interest of clear communication, I think we should also add tests for this new code before merging. After rebasing on main, you should be able to run invoke coverage on main and then again on this PR branch in order to compare the output and see which new lines need test coverage.

@justinmayer
Copy link
Member

@GiovanH: Two quick things…

  1. You'll need to update your local clone to reflect the default branch name change to main.
  2. When do you think this PR might be ready for review? I ask because I would like to release a new version of Pelican soon, and I think this would be a useful feature to include.

@GiovanH
Copy link
Contributor Author

GiovanH commented Jun 25, 2024

I just wanted to get a thumbs up from you on the feature first. I'll go ahead and write the tests/docs now.

@justinmayer
Copy link
Member

Ciao Giovanni. Just checking in… Any chance you could get this PR ready this weekend?

@GiovanH
Copy link
Contributor Author

GiovanH commented Jun 29, 2024

Thanks for the reminder, I'll see what I can do

@GiovanH
Copy link
Contributor Author

GiovanH commented Jun 30, 2024

I am continuing to have incredible difficulties with the build environment (as I always seem to with this project) so it might take a bit longer.

Renamed and rebased. Coverage looks good.

Because the nature of this feature is that articles marked "skip" are to be ignored entirely, I think the test updates may consist of just adding a skipped article and expecting tests to remain the same, like so:

@GiovanH
Copy link
Contributor Author

GiovanH commented Jun 30, 2024

I think I might actually not like skip as the status for this, because it's not parallel with the other statuses. Articles are drafts, articles are hidden, articles are published, but an article can't be "skip". "Disabled" would work, or "skipped" would force skip to be parallel with the existing enums.

@GiovanH GiovanH marked this pull request as ready for review June 30, 2024 05:03
@GiovanH GiovanH force-pushed the feature/disabled-status branch 2 times, most recently from 703f105 to 221973a Compare June 30, 2024 18:51
@justinmayer
Copy link
Member

Nice work, @GiovanH. Really appreciate your efforts.

I think I might actually not like skip as the status for this, because it's not parallel with the other statuses. Articles are drafts, articles are hidden, articles are published, but an article can't be "skip". "Disabled" would work, or "skipped" would force skip to be parallel with the existing enums.

Once we started overloading beyond Status: draft, we soon arrived at a point where none of these really make sense. "Disabled", "ignored", "skipped"… None of these have anything to do with the status of the article, but are instead what we are telling Pelican to do to the article. "Draft" works in this context, "hidden" kind of works, and… the rest IMO really don't.

Since "draft" is already taken, I say we come up with another status that implies unfinished, which Pelican then handles by ignoring it. Some potential candidates could be:

  • idea
  • plan
  • stub
  • sketch
  • outline
  • design

Personally, I like the first three options more than I like the last three.

@justinmayer
Copy link
Member

@GiovanH: On a side note, you previously said:

I am continuing to have incredible difficulties with the build environment (as I always seem to with this project) […]

Which specific difficulties have you had?

Which specific area(s) of the documentation about setting up the development environment are in need of improvement?

@GiovanH
Copy link
Contributor Author

GiovanH commented Jul 2, 2024

Once we started overloading beyond Status: draft, we soon arrived at a point where none of these really make sense. "Disabled", "ignored", "skipped"… None of these have anything to do with the status of the article, but are instead what we are telling Pelican to do to the article. "Draft" works in this context, "hidden" kind of works, and… the rest IMO really don't.

Since "draft" is already taken, I say we come up with another status that implies unfinished, which Pelican then handles by ignoring it. Some potential candidates could be:

  • idea
  • plan
  • stub
  • sketch
  • outline
  • design

Personally, I like the first three options more than I like the last three.

Of these, I think only stub works. The others aren't traditional statuses and don't clearly communicate that the document to be skipped.

In defense of skipped and disabled, I'd point to "hidden", which is also past-tense descriptive.

Pick whichever one you'd prefer out of stub, skip, skipped, or disabled. For the time being, I'll leave in disabled.

@GiovanH: On a side note, you previously said:

I am continuing to have incredible difficulties with the build environment (as I always seem to with this project) […]

Which specific difficulties have you had?

Which specific area(s) of the documentation about setting up the development environment are in need of improvement?

I think the documentation is fine, it's just that venv and invoke needed significant prodding to work in a cygwin environment. Invoke absolutely refuses to run outside a venv, and the script it installs didn't handle backslash escaping properly, so I had to run it with python3 -m invoke setup from within a venv. My guess is this just isn't a case invoke handles often enough to fix.

Most scripts that write files also use the "system default", and I have to mess with autocrlf or dos2unix to get the expected output.

@GiovanH GiovanH force-pushed the feature/disabled-status branch 3 times, most recently from 2ce3327 to cff9af1 Compare July 2, 2024 01:02
@justinmayer
Copy link
Member

Another status option I think fits this use case: incomplete

@GiovanH
Copy link
Contributor Author

GiovanH commented Jul 4, 2024

Another status option I think fits this use case: incomplete

I don't hate it, but I think it has the same problem of not clearly communicating the behavior. Drafts are incomplete articles too, but the point of the new status is to behave differently.

It's up to you though.

@GiovanH GiovanH force-pushed the feature/disabled-status branch 2 times, most recently from bba4c1f to f70ff5b Compare July 4, 2024 19:34
@GiovanH GiovanH changed the title Add disabled status for #3304 (POC) Add disabled status for #3304 Jul 4, 2024
@GiovanH
Copy link
Contributor Author

GiovanH commented Jul 25, 2024

Are you still looking to get this in soon?

@justinmayer
Copy link
Member

Hey @GiovanH. Thanks for your patience. I had intended on shipping a Pelican release last month, but my schedule took a hectic turn. My hope is to be able to dedicate some time to this in mid-August.

I am still conflicted about what status moniker to use for this, but given the discussion up to this point, I’m inclined to go with: skip

@GiovanH GiovanH force-pushed the feature/disabled-status branch 2 times, most recently from 2405a5a to ce09855 Compare July 27, 2024 01:06
@GiovanH
Copy link
Contributor Author

GiovanH commented Sep 12, 2024

@justinmayer Should be ready.

pelican/contents.py Outdated Show resolved Hide resolved
justinmayer and others added 2 commits September 12, 2024 11:14
Co-authored-by: Paolo Melchiorre <paolo@melchiorre.org>
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

Many thanks to @GiovanH for the excellent work on (and patience with!) the implementation of this feature. Thanks also to @pauloxnet for reviewing! ✨

@justinmayer justinmayer merged commit 4201256 into getpelican:main Sep 12, 2024
16 checks passed
@justinmayer justinmayer changed the title Add disabled status for #3304 Add skip status to ignore selected posts Sep 12, 2024
GeorgeHu6 added a commit to GeorgeHu6/pelican that referenced this pull request Sep 15, 2024
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.

Article status to gracefully ignore the file
3 participants