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

fix/release: fix comdirect bugs and update versioning #619

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

morremeyer
Copy link
Contributor

This PR contains two commits:

  • fix(ynap-parsers/de/comdirect): parsing of 'Ref.' field and thousands separator for comdirect
  • 🔖 publish new versions

The first one fixes two small bugs in the comdirect parsing.

It also changes the versioning in the lerna config to be independent, so that e.g. fixes for the parser can
be released without bumping the versions for the bank2ynab-converter and web-app packages.

This is an opinionated change, my reasoning here being that there is no need to release a new, unchanged version for packages that have not been changed since the last version.

The second PR bumps all versions since all packages have had changes since the last release. I ran npx lerna version for that commit.

@leolabs Is there a process to run the tests without needing to publish the packages first? I think to have the tests pass, right now, you would need to run lerna publish first which I obviously can not and even if I could, would not do right now.

Comment on lines -26 to +25
"ynap-parsers": "^1.15.0"
"ynap-parsers": "^1.15.1"
Copy link
Owner

Choose a reason for hiding this comment

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

We could consider switching the version to "*" which would use the latest available which might fix the current issue with builds where a bumped version isn't recognized properly. Alternatively, we could run yarn lerna bootstrap --production to locally link all packges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following https://lerna.js.org/docs/getting-started#package-dependency-management, we probably should not use lerna boostrap anymore. The --production flag has been removed, too.

I went with using yarn add ../ynap-bank2ynab-converter and yarn install for local testing, but that breaks the imports since it then requires the src directory to be in the import. (I'm no TS/JS expert, so this might be easy to solve in another way.

I see two ways here:

  1. Using a local install in the GitHub actions if that's possible (see above)
  2. Bump versions for separate packages only in separate PRs so they can be published separately and don't break tests for the others

I would much prefer the first solution if I'm honest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leolabs Do you have any ideas/input for this?

Copy link
Owner

Choose a reason for hiding this comment

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

I'd also go for the first one. Using workspaces in Yarn like they show in the docs sounds like a good option!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check if we can switch to yarn workspaces easily.

@leolabs
Copy link
Owner

leolabs commented May 10, 2023

Thanks for the PR! Builds are currently failing, but I think that isn't caused by this PR but rather something I missed in your last PR. See my comments above for what might be causing this.

Regarding your question, you should be able to run yarn lerna run test to run tests in all packages.

@morremeyer
Copy link
Contributor Author

Thanks for the feedback! I'll work on it later this week and will probably also add a bit of documentation.

Out of curiosity, I saw you closed all the renovate PRs, do you want to skip these updates or just clean up the PR list?

@leolabs
Copy link
Owner

leolabs commented May 10, 2023

Thanks! I just wanted to clean up the PR list a bit. Renovate can by quite noisy with their PRs 😅

@morremeyer
Copy link
Contributor Author

That is true. I can work on the configuration a bit or on more extensive testing so they are easier to merge - I do use renovate quite intensely and am very familiar with it. We could scope these things out in an issue?

I do like this project quite a bit for what it does and I'd love to help out more, especially since we plan on using it as a dependency for the Envelope Zero frontend.

@leolabs
Copy link
Owner

leolabs commented May 10, 2023

That sounds great! We might want to run the bank2ynab script again to get those importers up to date as well – this project has been stale for a while since I stopped using YNAB a few years ago and I didn't have sufficient time to give it the care it deserves.

@morremeyer
Copy link
Contributor Author

Moving the dependency update discussion to #640.

* refactor: use workspaces to enable testing of bumped versions

* test: add build & test workflow for web-app and bank2ynab-converter

* test: add results step to not need manual update of required checks when changing the matrix job
@morremeyer morremeyer force-pushed the fix/comdirect-ref-and-thousands branch from 4162ca7 to 1178927 Compare June 30, 2023 09:53
… separator for comdirect

With this commit, two bugs in the comdirect parsing are fixed.

It also changes the versioning in the lerna config to be independent, so that e.g. fixes for the parser can
be released without bumping the versions for the bank2ynab-converter and web-app packages.
 - ynap-bank2ynab-converter@1.14.1
 - ynap-parsers@1.15.1
 - ynap-web-app@1.15.1
@morremeyer morremeyer force-pushed the fix/comdirect-ref-and-thousands branch from 1178927 to e1df0a2 Compare June 30, 2023 09:55
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (89a4907) 91.09% compared to head (e1df0a2) 91.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
+ Coverage   91.09%   91.10%   +0.01%     
==========================================
  Files          25       25              
  Lines         865      866       +1     
  Branches      234      234              
==========================================
+ Hits          788      789       +1     
  Misses         70       70              
  Partials        7        7              
Impacted Files Coverage Δ
packages/ynap-parsers/src/bank2ynab/bank2ynab.ts 68.96% <ø> (ø)
...ackages/ynap-parsers/src/de/comdirect/comdirect.ts 96.42% <100.00%> (ø)
...ackages/ynap-parsers/src/util/read-encoded-file.ts 94.44% <100.00%> (+0.32%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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