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

Doc update - example on carmaker fixed #253

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

Doc update - example on carmaker fixed #253

wants to merge 24 commits into from

Conversation

hadrilec
Copy link
Contributor

example on carmaker fixed

@tfardet
Copy link
Collaborator

tfardet commented Feb 16, 2025

I'll try to have a look today afternoon or tomorrow

@tfardet
Copy link
Collaborator

tfardet commented Feb 17, 2025

@hadrilec I'm having a look now, in the meantime, could add py7zr to the pip install in the example test yaml file so we can try to get the tests passing?

Copy link
Collaborator

@tfardet tfardet left a comment

Choose a reason for hiding this comment

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

Thanks, it's indeed more readable!
I put a few remarks.
While you're at it, if you don't mind, could you remove the update=True from get_geodata('ADMINEXPRESS-COG-CARTO.LATEST:departement', update=True)?
Optionally maybe also the not so useful head()?

"metadata": {},
"outputs": [],
"source": [
"data.to_csv('sirene.csv')"
"init_conn(sirene_key='f7345356-8301-4567-b453-568301456723')"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should that really be there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, big mistake I will have to delete the PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

no, you have to reset the key, the data is leaked already ;)
the PR can stay, your key has to change ^^

Comment on lines +229 to +230
" else:\n",
" return pattern[0]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use a return early pattern, here: drop the else line and just keep return pattern[0]

Comment on lines +219 to +222
" match_list = ['RENAULT SAS', 'ALPINE', 'BATILLY', 'MAUBEUGE CONSTRUCTION',\n",
" 'TOYOTA', 'STELLANTIS AUTO SAS', 'RENAULT TRUCKS']\n",
" \n",
" values = ['RENAULT SAS', 'RENAULT SAS', 'RENAULT SAS', 'RENAULT SAS',\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

use a dictionary instead?

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.

2 participants