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

PEP8 standard #176

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

Conversation

Boyen86
Copy link

@Boyen86 Boyen86 commented Mar 10, 2023

This was originally done under the assumption that this file was part of our own codebase (Alliander).

In this PR I've updated the naming in the file to adhere to PEP8 standards. I'm not sure if that is the standard you are using at SIG, but if you are then this could be helpful.

In our own system I've also seperated modules. If there is interest I can create a PR for that as well, but for that these PEP8 changes will be required.

Please note, I don't think that creating forks is a good way for us to use this code. We would be much better off with a python library that we can call. We've made this request to your product manager at well.

@dennis-sig
Copy link
Contributor

Thanks for the merge request @Boyen86. At SIG we need to support a pretty huge variety of ecosystems, which also means that people use our client script in very different ways:

  1. The Docker container on DockerHub
  2. Via GitHub Marketplace
  3. By cloning this repository in their own pipeline, in Docker-based runners
  4. By cloning this repository in their own pipeline, in fixed runners
  5. By manually copying the script to their runners
  6. By forking it

The reason we (intentionally) don't use modules or pip is for scenario (5) and (6). This is mainly for people working with legacy technologies or ecosystems, but we also have to support those. We've had issues with people making mistakes in the manual copying, so that's where the whole just-copy-a-single-script approach comes from. If we can further reduce the number of clients on (5) and (6) we'll eventually split the script.

Could you elaborate on how you're using the client script? We actually discussed publishing this as a library when we originally introduced Sigrid CI, but we eventually abandoned those plans due to lack of interest. With more information about your setup we can probably do something to facilitate it better.

@Boyen86
Copy link
Author

Boyen86 commented Mar 13, 2023

Hey Dennis we are neutral towards making a fork or manually cloning the code in this repository. However, if and when we do that, we do expect the code to be in the same standard as the rest of our code. That's why I made the pull request for pep8, but to be completely honest there is more that I would want to see changed for the readability.

I don't think that is a viable route however, as many companies have their own standards in code. What we find readable might be viewed differently in another company. It doesn't really make sense to distribute the code, as only the interface is of interest. As such it seems like it would make much more sense to distribute this as a module/library.

As for our use case, we call this code from our python project that handles the import (so not a direct CLI call). An extra reason for use why a library that we could call makes more sense than the current implementation.

In the end, whether you call the functionality from the CLI or directly from a library is just a different way of interacting with the code. If the code could be split up between business logic and adapters (which really is a good practice in general) you only would need to add an additional entry point/adapter using the same business logic.

@nicorikken
Copy link
Contributor

Besides the discussion for publishing as a package on Pypi, it would already be an improvement if there would be a function to be called. Current we call the script using Python subprocess by providing arguments and environment parameters. This is a necessity as the script uses environment variables and reads arguments from how the script is called. My editor was also tripping over the code in the script, so making it pep8 is also a good improvement with this PR.

@dennis-sig
Copy link
Contributor

dennis-sig commented Mar 15, 2023

@nicorikken Potentially dumb question, but I assume you guys are direct colleagues?

We indeed expect the script to be called from the command line, the scenario where you integrate it within existing Python code is not really something we anticipated. I can imagine that's indeed not very convenient, since we heavily assume you're running it from the command line.

Regarding PEP-8: As you can probably tell we mostly use Java and TypeScript at SIG, and PEP-8 has some cop-out language that you can keep using camelCase if that was already the dominant style. That said, we don't have a strong opinion one way or the other. We intentionally kept everything in a single file, to make things easier for the people using scenario 5 in my list above, but we are experimenting with more output formats, which means more client-side logic, which necessitates a more typical module structure. That will already make the client script less of a command line script and more of a conventional Python project. We can coincide both the PEP-8 change and adding an entry point with those changes.

In terms of sequence:

  • Wait for the new end point to be available.
  • Merge this PR.
  • Split into modules and add additional export formats.
  • Provide code-level entry point.

@dennis-sig dennis-sig self-requested a review March 15, 2023 14:40
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.

3 participants