Skip to content

Comments

Feature/uploads#8

Open
lioseo wants to merge 4 commits intodev-v0.0.1from
feature/Uploads
Open

Feature/uploads#8
lioseo wants to merge 4 commits intodev-v0.0.1from
feature/Uploads

Conversation

@lioseo
Copy link

@lioseo lioseo commented May 18, 2025

add pipelines uploads

@lioseo lioseo requested review from duaghk, jeong2624 and swiri021 May 18, 2025 20:39
Copy link

Choose a reason for hiding this comment

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

In my opinion, pipeline_dir_path is more appropriate then pipeline_file_path.

Copy link

Choose a reason for hiding this comment

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

def allowed_file(filename):
return '.' in filename and
filename.rsplit('.', 1)[1].lower() in ALLOWED_EXTENSIONS

      is working good, but

def allowed_file(filename: Path):
try:
return filename.suffix.lower() in ALLOWED_EXTENSIONS
except:
return False

It will be more clear I think

Copy link

Choose a reason for hiding this comment

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

In this function:
if not os.path.exists(pipeline_dir):
os.makedirs(pipeline_dir)

but if pipeline_dir is Path:
pipeline_dir.mkdir(exists_ok=True, parents=True)
is more clear I think

Copy link

Choose a reason for hiding this comment

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

add .vscode is will helpful :)

Copy link

Choose a reason for hiding this comment

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

Set is unmutable type, but for clear(not confusing)
ALLOWED_EXTENSIONS = ('zip') is more proper class I think(tuple)

Copy link

Choose a reason for hiding this comment

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

Maybe, pipeline description can be nullable=True.

Copy link

@duaghk duaghk left a comment

Choose a reason for hiding this comment

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

All code was work well. :)

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