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

rename jansson pack/unpack-based Flux API functions #1104

Merged
merged 16 commits into from Jul 13, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Jul 10, 2017

This PR renames the following functions, as discussed in #1094

Old New
flux_kvs_lookup_getf() flux_kvs_lookup_get_unpack()
flux_msg_set_jsonf() flux_msg_pack()
flux_msg_vset_jsonf() flux_msg_vpack()
flux_msg_get_jsonf() flux_msg_unpack()
flux_msg_vget_jsonf() flux_msg_vunpack()
flux_rpcf() flux_rpc_pack()
flux_rpc_getf() flux_rpc_get_unpack()
flux_event_decodef() flux_event_unpack()
flux_event_encodef() flux_event_pack()
flux_request_decodef() flux_request_unpack()
flux_respondf() flux_respond_pack()

There will need to be a sched patch queued up before this should be merged, but wanted to get feedback first.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 10, 2017

I think the new function names are clearer.
Will it be slightly confusing that flux_rpc_get was replaced with flux_rpc_get_unpack, but flux_kvs_lookup_get is now flux_kvs_lookup_unpack (but still operates on a future)? It doesn't bother me too much but is the one thing that initially stood out.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 10, 2017

Coverage Status

Coverage increased (+0.004%) to 78.401% when pulling 5a4a001 on garlick:kvs_api_cleanup into 062ade5 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 10, 2017

Codecov Report

Merging #1104 into master will decrease coverage by 0.02%.
The diff coverage is 86.74%.

@@            Coverage Diff             @@
##           master    #1104      +/-   ##
==========================================
- Coverage   78.27%   78.24%   -0.03%     
==========================================
  Files         157      157              
  Lines       26132    26132              
==========================================
- Hits        20455    20448       -7     
- Misses       5677     5684       +7
Impacted Files Coverage Δ
src/common/libflux/panic.c 0% <0%> (ø) ⬆️
src/common/libkvs/kvs_lookup.c 80% <100%> (ø) ⬆️
src/common/libflux/rpc.c 93.75% <100%> (ø) ⬆️
src/common/libflux/barrier.c 84.84% <100%> (ø) ⬆️
src/common/libflux/flog.c 92.92% <100%> (ø) ⬆️
src/broker/log.c 68.32% <100%> (ø) ⬆️
src/broker/rusage.c 62.85% <100%> (ø) ⬆️
src/cmd/flux-module.c 84.49% <100%> (ø) ⬆️
src/broker/modservice.c 80.19% <100%> (ø) ⬆️
src/connectors/shmem/shmem.c 83.2% <100%> (ø) ⬆️
... and 47 more

@garlick garlick force-pushed the garlick:kvs_api_cleanup branch from 5a4a001 to ab0e327 Jul 10, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 10, 2017

Good point! Just forced a push that renames flux_kvs_lookup_unpack() to flux_kvs_lookup_get_unpack().

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 11, 2017

Coverage Status

Coverage decreased (-0.004%) to 78.393% when pulling ab0e327 on garlick:kvs_api_cleanup into 062ade5 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 11, 2017

sched builds fine with this branch - I misspoke when I said I thought a coincident PR was necessary, so this could be considered a merge candidate.

@garlick garlick force-pushed the garlick:kvs_api_cleanup branch from ab0e327 to 6d5f4c9 Jul 13, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 13, 2017

Just rebased this one after PR #1105 went in.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-0.004%) to 78.494% when pulling 6d5f4c9 on garlick:kvs_api_cleanup into afcbd94 on flux-framework:master.

garlick added some commits Jul 10, 2017

libkvs/lookup: rename getf->get_unpack
Rename flux_kvs_lookup_getf() to flux_kvs_lookup_get_unpack()
per discussion in #1094.

Update users in flux-kvs, libjsc, resource-hwloc module,
wrexecd, and KVS tests.
libflux/message: rename set_jsonf->pack
Rename flux_msg_set_jsonf() to flux_msg_pack()
per discussion in #1094.

Update unit tests.
libflux/message: rename vset_jsonf->vpack
Rename flux_msg_vset_jsonf() to flux_msg_vpack()
per discussion in #1094.

Update users in libflux message encoding functions.
libflux/message: rename get_jsonf->unpack
Rename flux_msg_get_jsonf() to flux_msg_unpack()
per discussion in #1094.

Update unit tests.
libflux/message: rename vget_jsonf->vunpack
Rename flux_msg_vget_jsonf() to flux_msg_vunpack()
per discussion in #1094.

Update users in libflux message decoding functions.
libflux/rpc: rename rpcf->rpc_pack
Rename flux_rpcf() to flux_rpc_pack() per discussion
in #1094.

Update users across flux-core, and unit tests.
libflux/rpc: rename rpc_getf->rpc_get_unpack
Rename flux_rpc_getf() to flux_rpc_get_unpack() per discussion
in #1094.

Update users across flux-core, and unit tests.
libflux/event: rename event_decodef->event_unpack
Rename flux_event_decodef() to flux_event_unpack()
per discussion in #1094.

Update unit tests and users in libflux, libjsc,
barrier module, and broker.
libflux/event: rename event_encodef->event_pack
Rename flux_event_encodef() to flux_event_pack()
per discussion in #1094.

Update unit tests and users in libflux, libjsc,
wreck job module, wrexecd, and broker.
libflux/request: rename decodef->unpack
Rename flux_request_decodef() to flux_request_unpack()
per discussion in #1094.

Update unit tests and users in libflux, broker,
and various modules.

@garlick garlick force-pushed the garlick:kvs_api_cleanup branch from 6d5f4c9 to 693a8b4 Jul 13, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.509% when pulling 693a8b4 on garlick:kvs_api_cleanup into 03ace30 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 13, 2017

Since there were no objections, I'll merge this once travis passes the latest rebase

@grondo grondo merged commit a3add43 into flux-framework:master Jul 13, 2017

4 checks passed

codecov/patch 86.74% of diff hit (target 78.27%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +8.46% compared to 03ace30
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 78.509%
Details

@garlick garlick deleted the garlick:kvs_api_cleanup branch Jul 13, 2017

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.