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.

Sign up for GitHub

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

Jump to bottom

TOML declarative parsing: 'general' section #2931

Merged
merged 3 commits into from
Nov 10, 2020
Merged

TOML declarative parsing: 'general' section #2931

merged 3 commits into from
Nov 10, 2020

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Nov 6, 2020 •
edited
Loading

First section parsed with the declarative specification

See commit messages for more details.

Key points:

  • type specs will be added when the spec format stops changing
  • new fields like 'mandatory_keys' might be added with the next sections
  • the support for imperative and declarative formats is temporary - when everything is declarative, the code will be simplified
  • export_all is also temporary

@codecov
Copy link

codecov bot commented Nov 6, 2020 •
edited
Loading

Codecov Report

Merging #2931 (2df5dfa) into toml-config (e90e8a8) will decrease coverage by 0.00%.
The diff coverage is 90.56%.

Impacted file tree graph

@@               Coverage Diff               @@
##           toml-config    #2931      +/-   ##
===============================================
- Coverage        79.16%   79.16%   -0.01%     
===============================================
  Files              375      376       +1     
  Lines            32600    32647      +47     
===============================================
+ Hits             25809    25845      +36     
- Misses            6791     6802      +11
Impacted Files Coverage Δ
src/config/mongoose_config_validator_toml.erl 96.45% <70.58%> (-0.94%) ⬇️
src/config/mongoose_config_spec.erl 80.95% <80.95%> (ø)
src/config/mongoose_config_parser_toml.erl 97.56% <98.52%> (-0.03%) ⬇️
src/pubsub/node_hometree.erl 77.77% <0.00%> (-5.56%) ⬇️
src/mod_bosh.erl 91.83% <0.00%> (-2.05%) ⬇️
src/mod_bosh_socket.erl 77.63% <0.00%> (-1.25%) ⬇️
src/global_distrib/mod_global_distrib_utils.erl 65.09% <0.00%> (-0.95%) ⬇️
src/ejabberd_c2s.erl 89.26% <0.00%> (-0.23%) ⬇️
src/mod_muc_log.erl 77.29% <0.00%> (ø)
... and 8 more

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 e90e8a8...2df5dfa. Read the comment docs.

@chrzaszcz chrzaszcz force-pushed the toml-declarative-general branch 3 times, most recently from 3a93dca to 0d7170c Compare November 9, 2020 09:08
@chrzaszcz chrzaszcz marked this pull request as ready for review November 9, 2020 10:07
arcusfelis
arcusfelis approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

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

looks good

@chrzaszcz
Declarative specification of the 'general' section
d6ac587 
- Temporary code for supporting both 'handler' specifications
- Type specs will follow when they stop changing
- Code for the decalrative handlers will be removed last

Functional changes:
- The 'general.mongooseim_access_commands.commands' option
  no longer accepts the string "all" for type consistency.
  Now this option has to be omitted to enable all commands.
janciesla8818
janciesla8818 approved these changes Nov 10, 2020
View reviewed changes
Copy link
Contributor

@janciesla8818 janciesla8818 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Good work with the initial work for the declarative approach, looks really promising.

Just one question, as we changed the general.mongooseim_access_commands.commands so it no longer accepts the string all could we update the docs accordingly or create another task to do so? I just want to make sure we don't forget about this.

@janciesla8818
Copy link
Contributor

💪 thanks for the additional changes.

@janciesla8818 janciesla8818 merged commit 78bdb4d into toml-config Nov 10, 2020
@janciesla8818 janciesla8818 deleted the toml-declarative-general branch November 10, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arcusfelis arcusfelis arcusfelis approved these changes

@janciesla8818 janciesla8818 janciesla8818 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrzaszcz @janciesla8818 @arcusfelis