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

Creates wrapper for endpoint /post/sale #12

Merged
merged 10 commits into from Oct 15, 2019
Merged

Creates wrapper for endpoint /post/sale #12

merged 10 commits into from Oct 15, 2019

Conversation

andremw
Copy link
Owner

@andremw andremw commented Oct 12, 2019

I'm still waiting for the guys @ eNotas to tell me what the possible values for "quandoEmitirNFe", "meioPagamento", "deducoes" and their corresponding meaning so that we create better types for it instead of using type number.

Closes #3

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
test/index.spec.ts Outdated Show resolved Hide resolved
@andremw andremw self-assigned this Oct 13, 2019
@guidiego
Copy link
Collaborator

@andremw I know that you will refactor my points! Take care, prefer use types than interfaces. use interface only to declare methods and types to declare properties.

Remember to assign a return type for every function.

nice job <3

@codecov-io
Copy link

codecov-io commented Oct 14, 2019

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #12   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      3    +2     
  Lines           2     26   +24     
  Branches        0      3    +3     
=====================================
+ Hits            2     26   +24
Impacted Files Coverage Δ
src/index.ts 100% <100%> (ø) ⬆️
src/sales.ts 100% <100%> (ø)
src/util.ts 100% <100%> (ø)

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 18eabe4...ae5be77. Read the comment docs.

@guidiego guidiego self-requested a review October 14, 2019 23:25
Copy link
Collaborator

@guidiego guidiego left a comment

Choose a reason for hiding this comment

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

Man I still thinking that is best we move sales stuff to src/sales and inside create types and index, but it's a minor! <3

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/api.ts Show resolved Hide resolved
orderBy: 0,
};

const api = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why u not use ApiInstance previously?

it('POST to create a sale', async () => {
const api = ({
post: jest.fn(),
} as unknown) as ApiInstance;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why as unkown as ApiInstance?

Copy link
Owner Author

@andremw andremw Oct 15, 2019

Choose a reason for hiding this comment

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

Because the compiler complains if I try to convert directly to ApiInstance, and it suggests that I first convert to unknown :(

post: jest.fn(),
} as unknown) as ApiInstance;

await createSale(api)({
Copy link
Collaborator

Choose a reason for hiding this comment

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

all this guys was required? I think that this kind of test should test only one think:

Throws when something is required
Set default values when we need

Copy link
Owner Author

Choose a reason for hiding this comment

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

We can either test it like this or export the mapSale function and write a test for it.. But since mapSale doesn't make sense as a "public" function I think we can keep testing createSale like this.

@andremw andremw merged commit 70bba33 into master Oct 15, 2019
@andremw andremw deleted the create-sale branch October 15, 2019 14:38
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.

Wrap POST /api/vendas
3 participants