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

Fix for spotify/ramlfications#85 #88

Closed
wants to merge 0 commits into from
Closed

Fix for spotify/ramlfications#85 #88

wants to merge 0 commits into from

Conversation

bpowell65536
Copy link

No description provided.

@codecov-io
Copy link

Current coverage is 97.11%

Branch #88 has no coverage reports uploaded yet.

Powered by Codecov. Updated on successful CI builds.

@econchick
Copy link
Contributor

woah 13 commits! I like a clean commit history (or as clean as possible) 😄 Please rebase/squash into one commit.

resources = create_resources(child.raw, resources, root, child)

return resources


def _create_supertype_resources(root, type_name, parent, supertype, resources):
Copy link
Contributor

Choose a reason for hiding this comment

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

I've used parent (and child) wording for nested resources; I think I'd rather call this parent types (and same with the parameter name)

@bpowell65536
Copy link
Author

Sorry, will do the squash/rebase. I'm a bit new to this stuff, so it might take me a while to figure out :)

if "type" in v:
# There may be some more methods defined on the resource
# type this one inherits from
supertype = v["type"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the _get(v, "type") approach here since it's used elsewhere. e.g.:

if _get(v, "type"):
    resources = _create_supertype_resources(root, k, parent, _get(v, "type"), resources)

@econchick
Copy link
Contributor

@bpowell65536 here's a good resource that I point people to on rebasing/squashing commits.

# There may be some more methods defined on the resource
# type this one inherits from
supertype = v["type"]
resources = _create_supertype_resources(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, do we actually do anything with resources here after it's set?

Copy link
Contributor

Choose a reason for hiding this comment

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

scratch that - I see it feeding into line 383

@econchick
Copy link
Contributor

@bpowell65536 once you squash everything (FYI it will require a force-push since you essentially re-write history when squashing) and address the little nitpicks, I'll merge this. This looks good, thank you for catching this bug and giving a patch!

@bpowell65536
Copy link
Author

@econchick Thanks! I have squashed everything now and pushed to my remote repository, but I'm not sure how to make the squashed commit appear instead of all the individual ones. Do I need to make a PR for the squashed commit instead?

@econchick
Copy link
Contributor

@bpowell65536 if you appropriately squashed everything, and (force) pushed to the same remote branch as before, then the pull request should automatically update. So I have a feeling it wasn't done correctly.

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