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

Fix the documentation warnings #3879

Merged
merged 5 commits into from
Jan 21, 2024
Merged

Conversation

Elv13
Copy link
Member

@Elv13 Elv13 commented Dec 30, 2023

They were added because the PRs which added those APIs predated the warnings, but were merged later.

And also fix some rendering bugs caused by the shims not behaving correctly.

Previously, things like `awful.screenshot` would print a false
positive warning because the `item.name` was fixed after the
linting rather than before it.
Those were written before the doc linting was merged.
They can now use multiple files. Some of the templates are getting
unmaintainable and would benefit from being multiple small widgets.
Previously, it would raw_set properties like floating. This means the
signals and the result of calling `awful.client.property.get` was
different. Some older code uses `awful.client.property.get` or
`awful.client.object.get_floating` rather than `c.floating`.

With this change, all native properties should be handled as so
and all non native properties set at the end, after `__newindex`
is defined.

The list of properties was extracted using

    cat client.c
        | tail -n100
        | grep -Eo '["][^"]+["]'
        | grep -Eo '[^"]+'
Copy link

codecov bot commented Dec 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cb72c0a) 91.01% compared to head (b5a5f7c) 91.02%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3879   +/-   ##
=======================================
  Coverage   91.01%   91.02%           
=======================================
  Files         901      901           
  Lines       57566    57609   +43     
=======================================
+ Hits        52392    52436   +44     
+ Misses       5174     5173    -1     
Flag Coverage Δ
gcov 91.02% <100.00%> (+<0.01%) ⬆️
luacov 93.72% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
lib/awful/screenshot.lua 95.05% <ø> (ø)
lib/gears/shape.lua 97.87% <ø> (ø)
...ests/examples/sequences/client/jump_to_urgent1.lua 100.00% <ø> (ø)
tests/examples/shims/beautiful.lua 100.00% <100.00%> (ø)
tests/examples/shims/client.lua 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@Elv13
Copy link
Member Author

Elv13 commented Jan 1, 2024

I will merge this soon because it's a doc only change and getting rid of those warning makes foxing other bugs easier

@actionless
Copy link
Member

They were added because the PRs which added those APIs predated the warnings, but were merged later.

mb then CI should fail on such warnings, to prevent it in the future?

@sclu1034
Copy link
Contributor

sclu1034 commented Jan 4, 2024

mb then CI should fail on such warnings, to prevent it in the future?

That would still not prevent such a PR from being merged, since CI runs on commit push, not on merge. Also known as "merge skew" or "semantic merge conflicts".
We'd also have to use something like GitHub's Merge Queues or bors.

@actionless
Copy link
Member

actionless commented Jan 4, 2024

runs on commit push, not on merge.

we had both before, when using Travis - but didn't moved to the current setup, idk if that was a purpose choice or just an overlook during the migration

@sclu1034
Copy link
Contributor

sclu1034 commented Jan 4, 2024

What Travis did doesn't help here. Travis does merge the newly pushed commits into the base branch, but the crucial part is, it does it at the time of the push. That still doesn't cover the issue here of

  1. Push to PR 1
  2. CI builds PR 1
  3. Push to PR 2
  4. CI builds PR 2
  5. Merge PR 2
  6. Merge PR 1 <-- here, the result from 2. is outdated, but there is no push to PR 1, so CI doesn't trigger again

Merge queues trigger at that 6.. At the time of the merge, they first merge head and base branch into a staging area, run CI on that, and then merge into the actual target branch. And they do those in sequence, even if two PRs are triggered to merge at the same time.

@Elv13 Elv13 merged commit e6f5c79 into awesomeWM:master Jan 21, 2024
11 checks passed
@Elv13 Elv13 deleted the 2023_xmas_commits_pr1 branch January 21, 2024 00:34
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.

3 participants