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 mapper #25

Closed
wants to merge 6 commits into from
Closed

Split mapper #25

wants to merge 6 commits into from

Conversation

lowkeypriority
Copy link
Contributor

@lowkeypriority lowkeypriority commented Oct 2, 2020

Description

I've been added split mapper and unit test for it.
Also added a description in README, but I don't know how to write an axample :\

Closes #24

"Roadmap"

  • splitMapper
    • Add file
    • Reexport in mappers/index.ts
    • Add unit test
    • Add section in README

@Siemienik
Copy link
Owner

Siemienik commented Oct 3, 2020

@lowkeypriority it's nice to see your PR 😸
I have two suggestions for you:

  1. could I ask you to do one issue in one PR? the fewer scope is much easier to review and maintain :) Additionally, it may be much quicker merge into master
  2. for the split mapper, may I recommend to create a factory? Something like that:
const factory = (options) => {
  const splitMapper = (value) => value.split(options.separator);

  splitMapper.separator = (separator) => factory({...options, separator});

  return splitMapper;
}

export const splitMapper = factory({separator: ','}); // create default split mapper

usage:

importer.GetAllItems({
  workshet:'Authors',
  type: ' list',
  columns:[
     {key:"fullname", index:1},
     {key: "books", index:2, mapper:splitMapper}, // map from `"book1,book2"` to `["book1","book2"]`
     {key: "adwards", index:3, mapper:splitMapper.separator(' && ')}, // map from `"2020 Adward && 2019 Adward"` to `["2020 Adward","2019 Adward"]`
  ]
});

@Siemienik Siemienik closed this Oct 3, 2020
@Siemienik
Copy link
Owner

ups.. It wasn't expected to close it... It was done automatically after #20 become merged

@Siemienik Siemienik reopened this Oct 3, 2020
@Siemienik Siemienik changed the base branch from refactoring-and-clean-up to master October 3, 2020 11:29
@lowkeypriority
Copy link
Contributor Author

@Siemienik thanks for review. I'll try to rewrite mappers to factory

Also I'll rename issue – one issue - one PR is food practice :)

@lowkeypriority lowkeypriority changed the title Adding string mappers Split mapper Oct 3, 2020
@lowkeypriority
Copy link
Contributor Author

@Siemienik Hello again.
I tried to implement the factory pattern but without success:

  1. I've created abstract class for mappers:
export abstract class Mapper {
  private options: object

  constructor(options: object) {
    this.options = options
  }

  abstract valueOf(value: string): string | string[]
}
  1. Made SplitMapper class extended from Mapper:
import { Mapper } from "./mapperFactory"

export class SplitMapper extends Mapper {
  constructor(options: object) {
    super({...options, separator: ','})
  }

  valueOf(value: string) {
    value.split(this.options.separator)
  }

  separator(separator: string) {
    this.options.separator = separator
  }
}

But this solution is not good at all - it contains TypeGuard bugs and, in my opinion, lacks a good abstraction.

Can you help with this? Any advice?

@Siemienik
Copy link
Owner

Siemienik commented Oct 5, 2020

@lowkeypriority code what I write works:
image

there is only lack of types.

As a mappers is used as lambda function matches to: (value: string) => any it isn't required to introduce classes and abstracts.

@Siemienik
Copy link
Owner

Siemienik commented Oct 5, 2020

Can you help with this? Any advice?

1. If I were doing this issue, probably I would like to create type ValueMapper<TResult> in src/config/ValueMapper.ts and use it in:

* https://github.com/Siemienik/xlsx-import/blob/master/src/config/IFieldConfig.ts#L5
* https://github.com/Siemienik/xlsx-import/blob/master/src/config/IColumnConfig.ts#L4
**EDIT: resolved in: #29 **

  1. Then create additional type: (above mapper implementation in same file like splitMapper)
  type SplitMapper = 
     ValueMapper<Array<string>>  // fulfil mapper behaviour 
     & {separator: (separator:string) => SplitMapper};` // adding api for customisations

@Siemienik
Copy link
Owner

Siemienik commented Oct 7, 2020

@lowkeypriority I've just updated the master branch. I added there the simplest example for a mapper (which essentially does nothing):

https://github.com/Siemienik/xlsx-import/blob/9a92510226a4c81d4d7030bc6632e9da8480e2d1/src/mappers/stringMapper.ts#L1-L3

and test for it:

https://github.com/Siemienik/xlsx-import/blob/9a92510226a4c81d4d7030bc6632e9da8480e2d1/tests/unit/mappers/stringMapper.test.ts#L1-L31

I suppose it will be handful for you 😸

@Siemienik Siemienik mentioned this pull request Oct 7, 2020
4 tasks
@Siemienik
Copy link
Owner

@lowkeypriority how do you up to? Is any way how can I help you?

@Siemienik
Copy link
Owner

suddenly I have to close this PR because lack of activity here, please open new one if needed

@Siemienik Siemienik closed this Oct 11, 2020
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.

Add splitMapper into mappers
2 participants