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

Feature/improvements #29

Merged
merged 6 commits into from
Jan 22, 2019
Merged

Conversation

RomainLanz
Copy link
Contributor

Hey 👋

Related issue: #28

This PR adds the following:

  1. Create an alias BaseTransformer;
  2. Rename toArray() to toJSON() (breaking change);
  3. Enable namespace resolving to define a transformer;
  4. Change defaultInclude & availableInclude methods to static getter (breaking change).

I need to go a little bit deeper into the code base to make the latest change (having multiple transform methods).

In the meanwhile, this can already be merged.

@codecov
Copy link

codecov bot commented Jan 21, 2019

Codecov Report

Merging #29 into master will decrease coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage     100%   99.78%   -0.22%     
==========================================
  Files          37       37              
  Lines         924      941      +17     
==========================================
+ Hits          924      939      +15     
- Misses          0        2       +2
Impacted Files Coverage Δ
test/serializers/data-serializer.spec.js 100% <ø> (ø) ⬆️
test/context.spec.js 100% <ø> (ø) ⬆️
test/promise.spec.js 100% <ø> (ø) ⬆️
test/available-includes.spec.js 100% <ø> (ø) ⬆️
test/serializers/plain-serializer.spec.js 100% <ø> (ø) ⬆️
test/exception.spec.js 100% <ø> (ø) ⬆️
test/meta.spec.js 100% <ø> (ø) ⬆️
test/eagerload.spec.js 100% <ø> (ø) ⬆️
test/includes.spec.js 95.91% <ø> (-4.09%) ⬇️
test/detault-includes.spec.js 100% <ø> (ø) ⬆️
... and 7 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 5d58b13...8a9b2c0. Read the comment docs.

@rhwilr
Copy link
Owner

rhwilr commented Jan 22, 2019

@RomainLanz you're awesome! 😃
I'll probably have a few minutes later today to take a look at it.

@rhwilr
Copy link
Owner

rhwilr commented Jan 22, 2019

Look great 👍 I've just one question and you're probably the right person to ask this anyway.

Regarding the BaseTransformer, what are the best practices for adonis packages and IoC aliases?
I think it would be better if packages would not pollute the global namespace and stay within Adonis\Addons\.... This way there is no confusion if BaseTransformer belongs to a package or is a class in the application.

I'd love to hear your opinion.

@RomainLanz
Copy link
Contributor Author

You are right.

Maybe we could create an aliase to Bumblebee\Transformer (or BumblebeeTransformer) or add a note in the README for this little tip.

@rhwilr
Copy link
Owner

rhwilr commented Jan 22, 2019

I like Bumblebee/Transformer. I'll rename the alias to this.

@rhwilr rhwilr merged commit 30f33ae into rhwilr:master Jan 22, 2019
@RomainLanz
Copy link
Contributor Author

Seems stable for me, I believe you can go ahead an release the version 2!

@RomainLanz RomainLanz deleted the feature/improvements branch March 20, 2019 20:31
@rhwilr
Copy link
Owner

rhwilr commented Mar 20, 2019

I've planned it for next week. I've to finish a thesis this week and then I'll finally have some more time for open-source 😄

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.

2 participants