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

Split header files #1570

Merged
merged 35 commits into from
Jan 25, 2018
Merged

Split header files #1570

merged 35 commits into from
Jan 25, 2018

Conversation

erszcz
Copy link
Member

@erszcz erszcz commented Nov 16, 2017

This PR splits some header files into more topic-specific ones: logging macros go to mongoose_logger.hrl, XMPP namespaces go to mongoose_ns.hrl (could we reuse the header from escalus/exml in the future?), some MAM specific records go to mod_mam.hrl.

It also makes a small step towards using only namespaced types throughout the codebase, by moving the definition of rsm_in() and rsm_out() from a header into jlib.erl and adding the module prefix in places where a global type was used before.

The motivation is that files which don't use parts defined in the big, generic header files don't include them anymore, which is nice when we want to reuse them outside the project.

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

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

These changes totally makes sense, thanks!

could we reuse the header from escalus/exml in the future?

I think yes, we could. On the other hand, this would mean, that when we need to add new namespace we would need to first update exml in order to use it in MongooseIM.

@michalwski
Copy link
Contributor

One more thing. All jobs on travis failed, this is probably not a random failure :)

@erszcz
Copy link
Member Author

erszcz commented Nov 20, 2017

Not at all :) I'm working on it.

@erszcz
Copy link
Member Author

erszcz commented Nov 22, 2017

Seems to be ok now :)

@avlindfors
Copy link
Contributor

avlindfors commented Jan 10, 2018

Have moved all errors defined as macros to separate module mongoose_xmpp_errors. Not sure about some function names because some are very similar but unrelated, this is however a quick fix if it is not acceptable.

@avlindfors
Copy link
Contributor

There are some macros that transform errors to binaries. These were often defined in multiple places but imo did not fit in the new mongoose_xmpp_errors module. They were moved to a new header file to reduce duplication instead (ejabberd_s2s.hrl).

Copy link
Member

@fenek fenek left a comment

Choose a reason for hiding this comment

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

  1. ?VERSION in mongoose.hrl is not fixed/removed.
  2. Not all mongoose_acc leftovers are removed from mongoose.hrl.
  3. jid() and similar definitions should be moved to jlib.erl
  4. Whole PR needs rebasing.

@@ -0,0 +1,5 @@
-record(scram,
{storedkey = <<"">>,
Copy link
Member

Choose a reason for hiding this comment

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

Strange indent.

@@ -0,0 +1,6 @@
-record(session, {sid,
Copy link
Member

Choose a reason for hiding this comment

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

sid should be in new line.

@@ -1,5 +1,9 @@
-include_lib("ejabberd/include/mod_privacy.hrl").

%% Used to get ?STREAM_TRAILER instead of making separate header file for it
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense to put it in jlib.hrl.

%% All errors are defined in mongoose_xmpp_errors.erl
%% Previously they were defined in up to three different modules

-define(INVALID_NS_ERR_TO_BIN,
Copy link
Member

Choose a reason for hiding this comment

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

Used only twice. We may get rid of the macro and replace it with exml:to_binary(mongoose_xmpp_errors:invalid_namespace())) in the code.

-define(INVALID_NS_ERR_TO_BIN,
exml:to_binary(mongoose_xmpp_errors:invalid_namespace())).

-define(HOST_UNKNOWN_ERR_TO_BIN,
Copy link
Member

Choose a reason for hiding this comment

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

Used only once, no need for macro at all.

-define(CONFLICT_ERR_TO_BIN,
exml:to_binary(mongoose_xmpp_errors:stream_conflict())).

-define(SOCKET_DEFAULT_RESULT, {error, badarg}).
Copy link
Member

Choose a reason for hiding this comment

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

Used only in s2s_out, no need to keep it here.

-define(INVALID_FROM_ERR_TO_BIN,
exml:to_binary(mongoose_xmpp_errors:invalid_from())).

-define(CONFLICT_ERR_TO_BIN,
Copy link
Member

Choose a reason for hiding this comment

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

Used only in ejabberd_service, no need for a macro.

-define(INVALID_XML_ERR_TO_BIN,
exml:to_binary(mongoose_xmpp_errors:xml_not_well_formed())).

-define(INVALID_FROM_ERR_TO_BIN,
Copy link
Member

Choose a reason for hiding this comment

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

Used only once in s2s_in, no need for a macro.

-define(HOST_UNKNOWN_ERR_TO_BIN,
exml:to_binary(mongoose_xmpp_errors:host_unknown())).

-define(INVALID_XML_ERR_TO_BIN,
Copy link
Member

Choose a reason for hiding this comment

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

Could be a special, separate function in mongoose_xmpp_errors, e.g. xml_not_well_formed_bin().

@avlindfors
Copy link
Contributor

Fixing/ removing ?VERSION from mongoose.hrl and rebase remains.

erszcz and others added 11 commits January 18, 2018 13:41
Change include directive in modules previously including ejabberd.hrl
Move type definitions to corresponding modules
Remove ejabberd URI
Change usage of constant to exported function-call
Remove usage of sm_session type in ejabberd_sm to session type
Move scram record definition to header file
Include scram header file in necessary modules
alexanderlindfors added 16 commits January 18, 2018 14:17
Uncertain about naming of some functions, especially related to stream errors
Local test failures seem unrelated to recent changes
Change all old usages of xmlel() to exml:element().
Change all old usages of iq() to jlib:iq()
Change usages of types to match new location
Change macro usages to function calls to mongoose_xmpp_errors
There are still error macro definitions defined in ejabberd_s2s_in/out and service
Move macros defined in multiple modules to new header file ejabberd_s2s.hrl
Minor style adjustments
Moved ?STREAM_TRAILER to jlib.hrl
Rebase and move new files to new structure
These types are now defined in jlib.erl
alexanderlindfors added 2 commits January 19, 2018 13:32
src/jlib.erl Outdated
-export_type([xmlel/0, xmlstreamstart/0, xmlstreamend/0, xmlstreamel/0,
-type iq() :: #iq{}.

-type user() :: binary().
Copy link
Member

Choose a reason for hiding this comment

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

It makes more sense to have all JID-related types defined in jid.erl.

}).

-record(external_component, {domain, handler, node}).
-include("mongoose_logger.hrl").

-define(DEPRECATED,
Copy link
Member

Choose a reason for hiding this comment

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

This is a mongoose_acc leftover as well.

@@ -136,13 +136,13 @@
id ,% :: mod_pubsub:nodeIdx(),
parents = [] ,% :: [mod_pubsub:nodeId(),...],
type = <<"flat">>,% :: binary(),
owners = [] ,% :: [jlib:ljid(),...],
owners = [] ,% :: [jlib::ljid(),...],
Copy link
Member

Choose a reason for hiding this comment

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

Why double :?

alexanderlindfors added 2 commits January 22, 2018 10:16
Create jid header file to hold jid record definition
@fenek fenek added this to the 3.0.0 milestone Jan 25, 2018
@fenek fenek merged commit 9419180 into master Jan 25, 2018
@fenek fenek deleted the split-header-files branch January 25, 2018 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants