Skip to content

More typed_kwargs for the gnome module#9594

Merged
eli-schwartz merged 6 commits intomesonbuild:masterfrom
dcbaker:submit/gnome-module-typed-kwargs-part-2
Dec 7, 2021
Merged

More typed_kwargs for the gnome module#9594
eli-schwartz merged 6 commits intomesonbuild:masterfrom
dcbaker:submit/gnome-module-typed-kwargs-part-2

Conversation

@dcbaker
Copy link
Member

@dcbaker dcbaker commented Nov 18, 2021

This is a smaller follow up to the last set of cleanups to the gnome module with conversions to typed_kwargs. There are only two functions covered here, genmarshal and generate_vapi. Again, this allows a number of nice cleanups. In addition I noticed that genmarshal still exposes build_always without deprecation. I've made that deprecated, and added the build_always_stale and build_by_default keyword arguments matching the behavior of custom_target.

@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #9594 (37a6fc0) into master (bc8c938) will increase coverage by 0.02%.
The diff coverage is 85.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9594      +/-   ##
==========================================
+ Coverage   67.06%   67.09%   +0.02%     
==========================================
  Files         404      404              
  Lines       85733    85681      -52     
  Branches    18910    18898      -12     
==========================================
- Hits        57501    57486      -15     
+ Misses      23684    23661      -23     
+ Partials     4548     4534      -14     
Impacted Files Coverage Δ
mesonbuild/interpreter/interpreter.py 83.12% <ø> (ø)
mesonbuild/modules/gnome.py 77.06% <84.44%> (+0.80%) ⬆️
mesonbuild/interpreter/type_checking.py 64.00% <100.00%> (+0.58%) ⬆️
mesonbuild/scripts/vcstagger.py 87.50% <0.00%> (-4.17%) ⬇️
interpreter/interpreter.py 82.21% <0.00%> (ø)
mesonbuild/build.py 77.20% <0.00%> (+0.21%) ⬆️
build.py 73.65% <0.00%> (+0.21%) ⬆️
interpreter/type_checking.py 64.00% <0.00%> (+0.58%) ⬆️
modules/gnome.py 76.39% <0.00%> (+0.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc8c938...37a6fc0. Read the comment docs.

@dcbaker
Copy link
Member Author

dcbaker commented Nov 18, 2021

The lgtm warning makes no sense, it thinks a function returning a tuple of 5 elements returns 4, then complains about it.

@dcbaker dcbaker force-pushed the submit/gnome-module-typed-kwargs-part-2 branch 2 times, most recently from e51624f to 0e5f0b0 Compare November 19, 2021 20:55
@dcbaker dcbaker force-pushed the submit/gnome-module-typed-kwargs-part-2 branch from 0e5f0b0 to db26fff Compare December 3, 2021 22:38
@mesonbuild mesonbuild deleted a comment from lgtm-com bot Dec 3, 2021
@dcbaker dcbaker force-pushed the submit/gnome-module-typed-kwargs-part-2 branch 2 times, most recently from 234703b to 5054911 Compare December 3, 2021 23:09
'gnome.genmarshal',
DEPEND_FILES_KW,
INSTALL_KW.evolve(name='install_header'),
KwargInfo('build_always', bool, default=False),
Copy link
Member

@eli-schwartz eli-schwartz Dec 7, 2021

Choose a reason for hiding this comment

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

There's a bunch of mysterious kwargs here, that are not in the documentation and are not in permittedKwargs. Attempting to use them raises a fatal error, e.g.

genmarshal/meson.build:1:0: ERROR: Got unknown keyword arguments "build_always"

In commit 80d665e, we added that error checking in, by following the documentation. At the time, the error message would be:

Meson encountered an error in file genmarshal/meson.build, line 1, column 0:
Invalid keyword argument build_always.

We also have some terrible manual argument checking in known_kwargs and known_custom_target_kwargs, but they've been entirely overridden for 4.5 years now, and no one has ever complained that they need these undocumented kwargs, which, again, were undocumented.

I see no particular grounds to re-add support for them half a decade later, especially just to mark at least one of them as deprecated in a later commit in the same PR... and if any of them do get added, that's fine, but they need to be added the same way kwargs usually get added, which means:

  • distinct commit adding them (and preferably explaining why they are useful)
  • I want them to be documented.
  • new kwargs should never be added in an already-deprecated state, so we can just skip "build_always" entirely.

@dcbaker dcbaker force-pushed the submit/gnome-module-typed-kwargs-part-2 branch 4 times, most recently from 973e910 to 9b7cdb0 Compare December 7, 2021 18:16
@lgtm-com
Copy link

lgtm-com bot commented Dec 7, 2021

This pull request introduces 1 alert when merging 9b7cdb0 into bc8c938 - view on LGTM.com

new alerts:

  • 1 for Unused import

@dcbaker dcbaker force-pushed the submit/gnome-module-typed-kwargs-part-2 branch from 9b7cdb0 to 9d04f61 Compare December 7, 2021 18:44
…ecking module

These are going to be used in the gnome module.
There are thee arguments that are passed directly to CustomTarget which
are not in the permittedKwargs: depends, depend_files, and build_always.
The first two are obviously generically useful, as they allow creating
correct ordering. The latter, not so much. Since it was an error to pass
it, we'll just delete it.
There is a change here, in that packages has error messaging for using
IncludeDirs objects in the packages argument, but it never worked, so
the message was useless.
@dcbaker dcbaker force-pushed the submit/gnome-module-typed-kwargs-part-2 branch from 9d04f61 to 37a6fc0 Compare December 7, 2021 18:50
@eli-schwartz eli-schwartz merged commit 824e091 into mesonbuild:master Dec 7, 2021
@dcbaker dcbaker deleted the submit/gnome-module-typed-kwargs-part-2 branch December 7, 2021 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants