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

Split core.py into two files in separate python package #1361

Merged
merged 9 commits into from
May 1, 2020

Conversation

heyman
Copy link
Member

@heyman heyman commented Apr 30, 2020

I took a stab at the second part of #1328 and split locust.core into a separate locust.user package containing two moduels; locust.user.task and locust.user.users, as described here: #1328 (comment).

I also moved the wait_time, sequential_taskset and inspectlocust modules (which I renamed to inspectuser) into the locust.user package.

When moving/renaming files I took extra care to have separate commits that would only move/rename a file without any changes, so that we will get to keep as much git history as possible.

Here is the current structure of the locust.user package:

user/
  - __init__.py
  - task.py
    - get_tasks_from_base_classes  (needed by both LocustMeta and TaskSetMeta)
    - @task
    - TaskSetMeta
    - TaskSet
    - SequentialTaskSet
    - DefaultTaskSet
  - users.py
    - LocustMeta
    - Locust
    - HttpLocust
  - wait_time.py
  - sequential_taskset.py
  - inspectuser.py

Overall I think it makes a lot of sense to have a user package namespace, though I'm a bit split when it comes to having the two separate user.task and user.users modules, since their code is very related to each other. I guess an alternative would be to recreate this PR, but only moving core.py into the user package, instead of splitting it into task.py and users.py.

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #1361 into master will increase coverage by 0.06%.
The diff coverage is 96.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1361      +/-   ##
==========================================
+ Coverage   80.70%   80.77%   +0.06%     
==========================================
  Files          24       26       +2     
  Lines        2254     2257       +3     
  Branches      345      344       -1     
==========================================
+ Hits         1819     1823       +4     
  Misses        343      343              
+ Partials       92       91       -1     
Impacted Files Coverage Δ
locust/user/wait_time.py 100.00% <ø> (ø)
locust/user/users.py 95.58% <95.58%> (ø)
locust/contrib/fasthttp.py 85.85% <100.00%> (ø)
locust/main.py 19.59% <100.00%> (-0.41%) ⬇️
locust/user/__init__.py 100.00% <100.00%> (ø)
locust/user/inspectuser.py 72.72% <100.00%> (ø)
locust/user/sequential_taskset.py 85.18% <100.00%> (ø)
locust/user/task.py 96.00% <100.00%> (ø)
locust/util/deprecation.py 75.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f80d63...b911299. Read the comment docs.

@cyberw
Copy link
Collaborator

cyberw commented May 1, 2020

Lgtm! Im ok with splitting user/task into separate files, it doesnt matter that much. Git is actually quite good at recognizing a rename+changes unless the changes are too big.

Ok to merge.

@heyman
Copy link
Member Author

heyman commented May 1, 2020

Git is actually quite good at recognizing a rename+changes unless the changes are too big.

Oh, I thought it only worked if the file renames/moves were made without any other changes to the file that was moved. That info might be outdated though, and maybe that has improved?

@heyman heyman merged commit 49225f4 into master May 1, 2020
@cyberw
Copy link
Collaborator

cyberw commented May 1, 2020

I dont know how small the changes need to be - Ive experienced it working and not working. Dont know if it has been improved :)

@cyberw
Copy link
Collaborator

cyberw commented May 1, 2020

You can see it before committing though

@heyman
Copy link
Member Author

heyman commented May 1, 2020

I dont know how small the changes need to be - Ive experienced it working and not working. Dont know if it has been improved :)

Ah! I guess doing a move/rename commit with no other changes should be super safe then :). Looking at the code for this PR, git does realise that user/task.py was previously core.py, but it hasn't kept the history for the lines of code that I moved out into user/users.py.

@heyman heyman deleted the core-restructure branch May 1, 2020 10:07
heyman added a commit that referenced this pull request May 1, 2020
…estructuring of core.py (#1361).

Change so that we refer to classes/functions through the `locust.*` namespace, since this will allow us to do other code restructuring changes without having to update docstrings and docs.
@cyberw
Copy link
Collaborator

cyberw commented May 1, 2020

Yea, I dont think git recognizes splitting a file in two.

@heyman
Copy link
Member Author

heyman commented May 1, 2020

Forgot to update documentation and docstrings for this PR. Fixed in 265a3bf.

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