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

allow defining parsers for non-digit tags #26

Merged
merged 1 commit into from
Jan 24, 2019

Conversation

prometh07
Copy link
Contributor

No description provided.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@bumi
Copy link
Contributor

bumi commented Jan 21, 2019

@Uepsilon what do you think?
for me this looks like a safe change. maybe we can add some specs for that parser?

Copy link
Contributor

@Uepsilon Uepsilon left a comment

Choose a reason for hiding this comment

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

specs would be nice, otherwise it's great!

@bumi
Copy link
Contributor

bumi commented Jan 22, 2019

which would probably be a spec to parse an example of such an MT940 file?

@Uepsilon Uepsilon merged commit be36556 into railslove:master Jan 24, 2019
@bumi
Copy link
Contributor

bumi commented Jan 25, 2019

@Uepsilon did you merge it now without specs?!

@Uepsilon
Copy link
Contributor

Uepsilon commented Jan 25, 2019 via email

@bumi
Copy link
Contributor

bumi commented Jan 25, 2019

ah, missed that. great! 🎉

@Uepsilon
Copy link
Contributor

i wasn't sure if i could have appended my specs to the PR of @prometh07 as it was coming from his fork. is that possible or what would have been the way to go here?

@bumi
Copy link
Contributor

bumi commented Jan 28, 2019

yep, pushing in the branch and thus updating the PR would be best I guess.

@Uepsilon
Copy link
Contributor

but i could not have pushed in that branch as it came from a fork i do not have write access to

@Uepsilon
Copy link
Contributor

could have mirrored the branch and then updated the PR

@prometh07
Copy link
Contributor Author

I was going to add some specs but you guys were faster. ;)

@Uepsilon
Copy link
Contributor

⚡️ 🏎 🏃 💨

:)

@bumi
Copy link
Contributor

bumi commented Jan 28, 2019

@Uepsilon typically (by default) it is enabled that maintainers have write access to the branch of the fork to update the PR.
So you could have pushed to prometh07:nonswift_fields and this PR would have been updated.
This is probably also a best practice as everything related to this PR is then documented here.

@Uepsilon
Copy link
Contributor

ah. this is amazing to know. i was not aware of that.. thanks @bumi, i'll check it next time

Uepsilon pushed a commit that referenced this pull request Jun 3, 2019
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

3 participants