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

Version parsers #274

Merged
merged 3 commits into from
Jul 27, 2020
Merged

Version parsers #274

merged 3 commits into from
Jul 27, 2020

Conversation

grant
Copy link
Member

@grant grant commented Jul 24, 2020

Proposed Changes

  • Simplify parsers
  • Remove duplicate code
  • Reduces file count/complexity over versioning

Description

@grant grant self-assigned this Jul 24, 2020
@grant grant requested a review from lance July 24, 2020 21:53
@grant grant changed the title Grant version parsers Version parsers Jul 24, 2020
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

This looks OK, but I'm trying to understand the benefit and where you have actually removed duplicate code. I see these three lines being consolidated by combining the two version files:

function parser(name: string, parser = new PassThroughParser()): MappedParser {
  return { name: name, parser: parser };
}

But I think that could addressed by moving that code into https://github.com/cloudevents/sdk-javascript/blob/master/src/parsers.ts and having both versions import it. They already have a dependency on the parsers file anyway. Personally, I prefer to have code for each version in a distinct file. Not sure how others feel about this.

@helio-frota
Copy link
Contributor

helio-frota commented Jul 27, 2020

@grant I'm not sure but my view on that is you are trying to "consolidate versions for the future" that approach is good so 👍 to that. So for what I understood when a new v.x spec come out, the project has the structure to add the things without much effort or something (?) also centralizing things in versions.ts ?

In other hand @lance's comment makes sense. So my question is your PR wording not accurate meaning or something ?

anyway I'm trying to get the point

@helio-frota
Copy link
Contributor

I prefer to have code for each version in a distinct file. Not sure how others feel about this.

@lance I have no idea how many times and how often new versions will be created/added. The approach looks good to me but not sure about the benefit related with the occurrence of the future changes/additions in versions.

👍

@grant
Copy link
Member Author

grant commented Jul 27, 2020

The PassThroughParser is not stateful and can be reused. It really should be static like I believe most every other function in this library.

Is there something you'd like me to change?

@lholmquist
Copy link
Contributor

I don't have any strong feelings on keeping things separate. ATM, since there are only 2 versions, it probably doesn't matter combining them.

@lance
Copy link
Member

lance commented Jul 27, 2020

Is there something you'd like me to change?

Not necessarily - was just trying to have some conversation around these questions.

@grant
Copy link
Member Author

grant commented Jul 27, 2020

To be clear, I saw a lot of improvements beyond this, but I'd never finish a concrete PR if I went chasing them.

@grant grant merged commit 3d82fb6 into cloudevents:master Jul 27, 2020
@grant grant deleted the grant_version_parsers branch July 27, 2020 17:25
@lance lance mentioned this pull request Jul 27, 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.

4 participants