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 332: to address column typo asper doc #461

Merged

Conversation

indiVar0508
Copy link
Contributor

Attempt to address #332 ,

based on API documentation parameters section asks for columns but class parameter accepts column.

added a deprecation warning for column with message to use columns instead

@lafrech
Copy link
Member

lafrech commented Sep 5, 2022

Thanks. This makes sense. I'm surprised this doesn't seem to trigger warnings in the tests.

Adding a test using the feature and one triggering the warning would be nice.

Please also add yourself to authors file.

@indiVar0508 indiVar0508 force-pushed the fix_332_address_docs_typo branch 2 times, most recently from ffda44b to 87e7ef8 Compare September 7, 2022 14:37
Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

Thanks. Just a minor comment.

src/marshmallow_sqlalchemy/fields.py Outdated Show resolved Hide resolved
Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

Thanks.

@lafrech lafrech merged commit 4f8ac18 into marshmallow-code:dev Sep 7, 2022
@lafrech
Copy link
Member

lafrech commented Sep 7, 2022

I don't have permissions to push to dev branch.

@sloria, if you pass by, here's the changelog I was writing.

0.29.0 (2022-09-07)
+++++++++++++++++++

Features:

* Rename ``column`` argument of field ``marshmallow_sqlalchemy.fields.Related``
  into ``columns`` to match the API documentation. Deprecate the use of
  ``column``. (:issue:`332`)
  Thanks :user:`bbbart` for reporting and :user:`indiVar0508` for the PR.

@sloria
Copy link
Member

sloria commented Sep 7, 2022

@lafrech changelog looks good to me.

I don't have permissions to push to dev branch.

odd, i just checked the repo settings and i don't see anything that should block you. i bumped you up to have Maintain privs. can you try again?

@lafrech
Copy link
Member

lafrech commented Sep 8, 2022

I'm one commit ahead of dev and I try to push.

* 8031311 2022-09-07 | Bump version and update changelog (HEAD -> dev) [Jérôme Lafréchoux]
*   4f8ac18 2022-09-07 | Merge pull request #461 from indiVar0508/fix_332_address_docs_typo (origin/dev, origin/HEAD) [Jérôme Lafréchoux]
$ LANG=EN git push
Enumerating objects: 11, done.
Counting objects: 100% (11/11), done.
Delta compression using up to 2 threads
Compressing objects: 100% (5/5), done.
Writing objects: 100% (6/6), 732 bytes | 732.00 KiB/s, done.
Total 6 (delta 4), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (4/4), completed with 4 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/dev.
remote: error: Required status check "marshmallow-code.marshmallow-sqlalchemy" is expected.
To github.com:marshmallow-code/marshmallow-sqlalchemy.git
 ! [remote rejected] dev -> dev (protected branch hook declined)
error: failed to push some refs to 'github.com:marshmallow-code/marshmallow-sqlalchemy.git'

We've seen this already and I don't remember what solved it.

Looks like a branch protection issue. I don't think I have access to that in the settings. I'm surprised that the status of maintainer doesn't let me bypass this, though.

(I didn't mean to become a maintainer, but since I use this lib I thought I'd do what I can to help so I use my merge permission to help merging automatic and occasional PRs. I don't know the code enough to provide real contributions.)

@lafrech lafrech mentioned this pull request Feb 8, 2023
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

3 participants