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

feat: add scaleway provider #1105

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

fuse
Copy link

@fuse fuse commented Feb 6, 2025

Generate associated code using autogen.sh

Generate associated code using `autogen.sh`
Copy link

@davidaparicio davidaparicio left a comment

Choose a reason for hiding this comment

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

LGTM

@gabriel-tessier
Copy link
Collaborator

gabriel-tessier commented Feb 11, 2025

@fuse
Hi, and thanks for the PR.
Can you check the contributing file, more especially the add new provider section and validate that all the step are done.

https://github.com/mingrammer/diagrams/blob/master/CONTRIBUTING.md#add-new-provider

I quickly check the changes and there's docs/nodes/gis.md that is updated can you revert at least this one to only have the changes related to add Scaleway provider. <--- this comment was edited look like the change in the file is somewhere else you can ignore.

Just small comments :

  • About the size of the icons, in the doc it's minimum 256 px, may need to resize your icons.
  • About the naming all files start with capital case which generate node name like this: Accountexperience
    and in your code you will go with
from diagrams.scaleway.account import Accountexperience

Should be better like this:

from diagrams.scaleway.account import AccountExperience

Also you can use alias. If you have questions or things that you don't get, just ask, we are here to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feat/provider Provider request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants