Fix issue #2. Add singer message support #10
Conversation
|
I've changed the readme because the target is not working without a virtualenv. |
|
Closing this. Not enough support by the maintainers. A pity, singer seems like it could evolve to a great project in the long run. Best regards. |
|
LOL. This is awesome. Thank you! :) |
| installation instructions for [Mac](python-mac) or | ||
| [Ubuntu](python-ubuntu). | ||
| installation instructions for [Mac] or | ||
| [Ubuntu]. |
There was a problem hiding this comment.
Whoa. Was not aware of this Markdown feature. Is this GFM or Markdown proper? (Linking to a named ref without the trailing []).
There was a problem hiding this comment.
Actually is just proper Markdown. Check this out https://github.com/adam-p/markdown-here/wiki/Markdown-Here-Cheatsheet#links
There was a problem hiding this comment.
Sorry I hadn't seen the closed PR already! And sorry it took us so long to get to. FWIW Stitch has been changing how we managed open source activity internally of late so hopefully you should see much faster turn around times for contributions like this.
Back to you! :)
README.md
Outdated
|
|
||
| ```bash | ||
| > virtualenv -p python3 venv | ||
| > source venv/bin/activate |
There was a problem hiding this comment.
Let's do;
python3 -m venv ~/.virtualenvs/target-csv
source ~/.virtualenvs/target-csv/bin/activate
pip install -U pip setuptools
pip install -e '.[dev]'
There was a problem hiding this comment.
Note that those are without the leading > . Makes it easier to copy/paste. :)
target_csv.py
Outdated
| @@ -1,5 +1,3 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Need to keep the hashbang since the file's executable, although it could be changed to python.
| input_messages = io.TextIOWrapper(sys.stdin.buffer, encoding='utf-8') | ||
| state = persist_messages(config.get('delimiter', ','), | ||
| config.get('quotechar', '"'), | ||
| input_messages) |
There was a problem hiding this comment.
Can you provide a gist showing that the output hasn't changed between this and master? That way the refactoring is nice and solid.
There was a problem hiding this comment.
Actually if you install target-csv before a new tap, like tap-exchangeratesapi, it crashes, because target-csv now requires a specific outdated version of singer-python to work. I changed the code in setup.py to fix the problem. What do you think?
There was a problem hiding this comment.
Ah. This comes up fairly regularly and I'm not sure exactly how to make it clearer. As is outlined in the Running and Developing guide, the intended way to run taps and targets is in separate virtual environments. This avoids issues exactly like you're describing. Our official stance on loose version requirements (as official as a comment on an issue can get!) shows our Java roots but we think ultimately yields a more sane development experience. :)
So my recommendation would be to reverse the setup.py change and maybe link to that getting started guide?
There was a problem hiding this comment.
Ok, done! I think the development experience may be a bit more sane, but for newcomers to the library is very difficult to use. I mean, a lot of python users don't even know how to use a single virtualenv, imagine using two, one for each library. The end user just suffer more than it is necessary in my opinion. Just food for thought.
Do you think it is necessary to show a gist?
|
@joaomarcosgris Thanks for this! I've added an issue to update the README to clarify some of the virtual environment stuff (and update it to use I'm running a quick local test with this and will be good to merge this PR after. |
dmosorast
dismissed
timvisher’s stale review
Ran through a local test and confirmed state was written appropriately. No gist necessary.
|
Looks good! Merging. Thank you! Addresses #2 |
Also, I've removed some trailing whitespaces and made the code more compliant with pep8.