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

Adds a tool for inspect that checks for character limits #1683

Merged
merged 6 commits into from Aug 18, 2015

Conversation

Bcorde5
Copy link
Contributor

@Bcorde5 Bcorde5 commented Jul 22, 2015

The first test is to see how circle tests handles the build, if successful, then I will fix the mistakes. Comments and criticisms are welcome.

@K-ballo
Copy link
Member

K-ballo commented Jul 22, 2015

Would it be possible to except #error directives from this check?

@Bcorde5
Copy link
Contributor Author

Bcorde5 commented Jul 22, 2015

I suppose, let me work on it, and as long as that is the preferred result it could work.

@sithhell
Copy link
Member

I think it would be a good idea to exclude these items:

  • CMakeLists.txt
  • docs
  • external source code

I am also not a big fan of a strict 80 characters line limit. I'd rather prefer to escape this limit occasionally. It's sometimes odd to just break a line because you are a few characters over the limit.

@hkaiser
Copy link
Member

hkaiser commented Jul 23, 2015

I am also not a big fan of a strict 80 characters line limit. I'd rather prefer to escape this limit occasionally. It's sometimes odd to just break a line because you are a few characters over the limit.

Agreed. We settled on a hard 90 character limit for now (with 80 characters being a 'soft' limit as before). The main incentive for this rule is to ensure readability in any contexts (i.e. in diff-views on github, etc.)

@hkaiser
Copy link
Member

hkaiser commented Jul 23, 2015

I think it would be a good idea to exclude these items:

  • CMakeLists.txt
  • docs
  • external source code

Sure, that can be done. Even if I think that files under our control should be slowly fixed to conform to this rule as it simplifies online reading, etc. For external code inspect can be disabled by simply placing a file named hpx-no-inspect into the base directory of the sub-tree which should not be checked.

@Bcorde5
Copy link
Contributor Author

Bcorde5 commented Jul 23, 2015

Yeah, the name of the branch was made before we agreed on the limit, the actual limit is set upon launch, with the default being 90.

Should I exempt links (https://...) from the limit?

@Bcorde5
Copy link
Contributor Author

Bcorde5 commented Jul 27, 2015

There seems to be a problem with giving circleci the option to choose the length of the limit, so for now it is going to be set at 90, only configurable in the inspect tool. Please let me know if you still want to prompt circleci to set the limit.

@hkaiser
Copy link
Member

hkaiser commented Jul 27, 2015

There seems to be a problem with giving circleci the option to choose the length of the limit, ...

@Bcorde5 Sorry, I don't understand what you mean by 'there seems to be a problem'.

@Bcorde5
Copy link
Contributor Author

Bcorde5 commented Jul 28, 2015

Disregard that last statement, I believe I have found a solution to the problem.

@Bcorde5 Bcorde5 force-pushed the 80_line_limit branch 3 times, most recently from 46c34da to 07be225 Compare July 29, 2015 19:00
@Bcorde5
Copy link
Contributor Author

Bcorde5 commented Aug 10, 2015

I am going to make an exception for #define and #if right now, if this seems troublesome, please let me know soon.

Created to default to hard limit of 90, excludes documented files, CMakeLists.txt, and #error.
@hkaiser
Copy link
Member

hkaiser commented Aug 10, 2015

I am going to make an exception for #define and #if right now, if this seems troublesome

Preprocessor directives have to be one a single line. However, you can use a single \ at the end of the line to introduce line breaks into those:

#define FOO \
    'this is what FOO expands to'

@Bcorde5
Copy link
Contributor Author

Bcorde5 commented Aug 11, 2015

Now that I have the grasp of how to use , I can fix #define and #if, so only #error will be an exception, thank you.

@Bcorde5
Copy link
Contributor Author

Bcorde5 commented Aug 12, 2015

It has been decided that url links will be ignored by this check, as it would be too confusing if they were edited.

@Bcorde5
Copy link
Contributor Author

Bcorde5 commented Aug 13, 2015

Does anyone know what is causing the error?

@hkaiser
Copy link
Member

hkaiser commented Aug 13, 2015

@Bcorde5 just look at the circleci log here: https://circleci.com/gh/STEllAR-GROUP/hpx/1318

…ine_limit

Conflicts:
	examples/1d_hydro/1d_hydro_upwind.cpp
	examples/transpose/transpose_block.cpp
	hpx/lcos/detail/full_empty_entry.hpp
	hpx/parallel/executors/service_executors.hpp
	hpx/runtime/threads/policies/static_priority_queue_scheduler.hpp
	hpx/runtime/threads/threadmanager.hpp
	src/hpx_init.cpp
	src/runtime/threads/executors/thread_pool_executors.cpp
	src/runtime_impl.cpp
	tests/performance/local/vector_foreach.cpp
…ine_limit

Conflicts:
	hpx/plugins/parcel/coalescing_message_handler.hpp
	hpx/plugins/parcel/message_buffer.hpp
	hpx/runtime/parcelset/decode_parcels.hpp
	hpx/runtime/parcelset/parcel.hpp
	hpx/runtime/parcelset/parcelhandler.hpp
	hpx/runtime/parcelset/parcelport_impl.hpp
	plugins/parcelport/mpi/parcelport_mpi.cpp
	src/runtime_impl.cpp
hkaiser added a commit that referenced this pull request Aug 18, 2015
Adds a tool for inspect that checks for character limits
@hkaiser hkaiser merged commit 05a7a86 into STEllAR-GROUP:master Aug 18, 2015
@hkaiser
Copy link
Member

hkaiser commented Aug 18, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants