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

Stats improvements #465

Merged
merged 10 commits into from Jan 7, 2022
Merged

Stats improvements #465

merged 10 commits into from Jan 7, 2022

Conversation

idomic
Copy link
Contributor

@idomic idomic commented Jan 5, 2022

No description provided.

@idomic idomic requested a review from edublancas January 5, 2022 15:30
@idomic
Copy link
Contributor Author

idomic commented Jan 5, 2022

Once OKed I'll squash the commits

author idomic <michael.ido@gmail.com> 1641325888 -0500
committer idomic <michael.ido@gmail.com> 1641401611 -0500

# This is a combination of 5 commits.
# This is the 1st commit message:

Working v2 + Tests

# This is the commit message #2:

Added tests

# This is the commit message #3:

linting

# This is the commit message #4:

Pulled from master

# This is the commit message #5:

Rebased

Pulled from master
src/ploomber/telemetry/telemetry.py Show resolved Hide resolved
src/ploomber/telemetry/telemetry.py Outdated Show resolved Hide resolved
src/ploomber/telemetry/telemetry.py Outdated Show resolved Hide resolved
src/ploomber/telemetry/telemetry.py Outdated Show resolved Hide resolved
tests/telemetry/test_telemetry.py Outdated Show resolved Hide resolved
tests/telemetry/test_telemetry.py Outdated Show resolved Hide resolved
def check_first_time_usage():
config_path = os.path.join(check_dir_file_exist(CONF_DIR), 'config.yaml')
uid_path = os.path.join(check_dir_file_exist(CONF_DIR), 'uid.yaml')
return not os.path.exists(uid_path) and os.path.exists(config_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be not os.path.exists(uid_path) and not os.path.exists(config_path)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No since the call is being performed after the check for a stats config file (check the calling function)

src/ploomber/telemetry/telemetry.py Outdated Show resolved Hide resolved
src/ploomber/telemetry/telemetry.py Outdated Show resolved Hide resolved
uid = str(uuid.uuid4())
try: # Create for future runs
with open(uid_path, "w") as file:
with uid_path.open("w", encoding="utf-8") as file:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we leave the default encoding instead of hardcoding? this is system-dependent. no need to change it but for future reference you can use Path.write_text and Path.read_text instead of Path.open

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, where I can I'll try these functions, here I figured it's better to enforce a yaml structure via the class, without open it will not work. You can take a look of this thread: https://stackoverflow.com/questions/57296168/pathlib-path-write-text-in-append-mode

src/ploomber/telemetry/telemetry.py Outdated Show resolved Hide resolved
@edublancas
Copy link
Contributor

edublancas commented Jan 6, 2022

what happens if any of the functions that check the config fail? like get_uid? there is a lot of read/write logic in there which may fail due to filesystem permissions or any other thing. that should not stop the user from using the tool.

So maybe have something like this:

def _check_uid():
    # all the logic here
     pass

def check_uid():
    try:
        return _check_uid():
    exception Exception:
        return None

thoughts?

@edublancas edublancas merged commit 94fcf4a into master Jan 7, 2022
@edublancas edublancas deleted the stats_improvements branch January 7, 2022 01:27
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.

None yet

2 participants