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

Refactor to keep feature simple #36

Merged
merged 8 commits into from Aug 31, 2018
Merged

Conversation

icyleaf
Copy link
Owner

@icyleaf icyleaf commented Aug 31, 2018

So sorry to refactor, i hope you are not use unstable master branch.

Feature:

  • Halite::Features::Logger to Halite::Logger
  • Halite::Features.register to Halite.register_feature
  • Halite::Features[]/[]? to Halite.features/features?
  • Halite::Features.avaiables to Halite.has_feature?
  • Halite::Feature::Interceptor::Chain to Halite::Feature::Chain

Logger

  • Rename Halite::Logger to Halite::Logging to better understand
  • Rename feature name "logger" to "logging"

This changes was not impact the usage with .logger method in Chainable module.

MimeType

  • Rename Halite::MimeTypes to Halite::MimeType
  • Merge Halite::MimeTypes.register_adapter/register_alias into Halite::MimeType.register

…ture under Halite and some method changes.

Feature:

- `Halite::Features::Logger` to `Halite::Logger`
- `Halite::Features.register` to `Halite.register_feature`
- `Halite::Features[]/[]?` to `Halite.features/features?`
- `Halite::Features.avaiables` to `Halite.has_feature?`
- `Halite::Feature::Interceptor::Chain` to `Halite::Feature::Chain`
@ghost ghost assigned icyleaf Aug 31, 2018
@ghost ghost added the in progress label Aug 31, 2018
@icyleaf icyleaf force-pushed the refactor/keep-feature-simple branch from d06f394 to 1ec66b8 Compare August 31, 2018 03:43
This was referenced Aug 31, 2018
…d merge Halite::MimeTypes.register_adapter/register_alias into Halite::MimeType.register
@icyleaf icyleaf force-pushed the refactor/keep-feature-simple branch from 918e07e to e46f84d Compare August 31, 2018 06:07
@j8r
Copy link

j8r commented Aug 31, 2018

I found the name Halite::Logger fine, line in the stdlib https://github.com/crystal-lang/crystal/blob/master/src/logger.cr (the name is also shorter)

The main concern for me was the extensive use of class variables, particularly in modules.
I haven't tested yet, but there are good chances this variables are shared across all Halite Objects.
We may want to have independent Objects when spawning Fibers for example.
For example one that log to STDOUT, the other at the same time downloading in the background and log to a file.

What do you think, is it the desired behavior?

@j8r
Copy link

j8r commented Aug 31, 2018

Also a thought, what do you think of defining features and behaviors in properties in a Halite or a Halite::Client Object instead of a Hash? This would be quite a lot more efficient, and may solve the above issue.

@icyleaf
Copy link
Owner Author

icyleaf commented Aug 31, 2018

I tried use feaure class as argument, but it will be complex, eg:

Halite.use(Halite::Logging.new(logger: Halite::Logging::JSON.new(skip_response_body: true)))

@icyleaf
Copy link
Owner Author

icyleaf commented Aug 31, 2018

We may want to have independent Objects when spawning Fibers for example.
For example one that log to STDOUT, the other at the same time downloading in the background and log to a file.

This only apply in monitoring case, if some feature changed value of request, we can not apply it to the other featues when spawning.

Copy link
Owner Author

@icyleaf icyleaf left a comment

Choose a reason for hiding this comment

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

It Seems ready to merge.

@icyleaf icyleaf merged commit 596469d into master Aug 31, 2018
@ghost ghost removed the in progress label Aug 31, 2018
@icyleaf icyleaf deleted the refactor/keep-feature-simple branch August 31, 2018 09:53
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.

None yet

2 participants