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

Anbima calendar #726

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

davimmilhome
Copy link

@davimmilhome davimmilhome commented Oct 26, 2022

refs #

For information, read and make sure you're okay with the Contributing guidelines.

  • [v ] Tests with a significant number of years to be tested for your calendar.
  • [v ] Docstrings for the Calendar class and specific methods.
  • [v ] Use the workalendar.registry_tools.iso_register decorator to register your new calendar using ISO codes (optional).
  • [v] Calendar country / label added to the README.md file.
  • [v ] Changelog amended with a mention like: "Added <country> by @pseudo (#)". Note Please do NOT change the version number here. It's the project maintainers' duty.
  • Tests with a significant number of years to be tested for your calendar.
  • Changelog amended with a mention describing your changes. Note Please do NOT change the version number here. It's the project maintainers' duty.

Copy link
Member

@brunobord brunobord left a comment

Choose a reason for hiding this comment

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

This is a very interesting contribution, but you have committed several files that should not belong to this pull request:

  • every file in th .idea/ directory
  • the README.md.bak file

also, I'd like to know what's this "pytest.mark.anbima_test" marker?

@@ -1009,3 +1009,71 @@ def test_no_duplicate(self):
def test_all_are_brazilian_classes(self):
for key, value in IBGE_TUPLE:
self.assertTrue(issubclass(value, Brazil))

@pytest.mark.anbima_test
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Author

Choose a reason for hiding this comment

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

ill corect this

@@ -1,7 +1,10 @@
from datetime import date

from ..america import (Argentina, Barbados, Chile, Colombia, Mexico, Panama,
Paraguay)
Paraguay,)
Copy link
Member

Choose a reason for hiding this comment

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

what's that change here? Brazil has its own test module...

Copy link
Author

Choose a reason for hiding this comment

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

Actually i see this before, that brazil has it own module. I started to code on america, but i cut all to brazil module. This edit in truth dont change anything.

Copy link
Author

Choose a reason for hiding this comment

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

@brunobord, i think that i solve the problems can you check again ? Thanks for instruct me, this is my first pr

@davimmilhome
Copy link
Author

I think that is nice now, i need do something more?

Copy link
Member

@brunobord brunobord left a comment

Choose a reason for hiding this comment

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

as you may see, you've got several errors in your code.

I'd recommend you to try to solve them by running the tests locally first, and fix them step by step. Your main problem apparently comes from the line 633 in the workalendar/america/brazil.py file, in which you're trying to use the workalendar.america.Brazil class, while the root module of this namespace (namely workalendar has not been defined or imported).

I think you'd rather put just Brazil here, instead. This class is defined in this very file, so it's already loaded in the current namespace.

This change should at least remove several errors. There might be others.

If you need help on how to fix your PR, please don't hesitate to ask.

Changelog.md Outdated
@@ -4,6 +4,10 @@

Nothing here yet.

## v17.1.0 (2023-01-02)
Copy link
Member

Choose a reason for hiding this comment

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

please, remove this "release number" here, you'd rather add the changelog entry to the current (master) section.

FYI, it's in the Github PR template, as well as in the "How to Contribute" documentation: https://github.com/workalendar/workalendar/blob/master/docs/contributing.md#the-final-steps

@davimmilhome
Copy link
Author

davimmilhome commented Jan 9, 2023

Yeah man, sory for the preview code, i was learning. I corect the tests now, maybe fine now. About changelog i rly dont know what mean the # with three numbers.

Hope you're doing well guys and Bruno!

@davimmilhome
Copy link
Author

I think that im messing up with some config in my setup that generate so many unecessary files from commit, maybe a pycharm feature, i have to search about it, but at time, i exclude all this files manually

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